Implemented screen notifying users about malformed HWID.

Implemented screen notifying users about malformed HWID.

UI tests will be implemented later.

TBR=ben
BUG=137121
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=181941

[Nikita (slow), 2013-02-11 17:09:03]

Generally looking good, mostly nits.

https://codereview.chromium.org/12213110/diff/6002/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/DEPS (right):

https://codereview.chromium.org/12213110/diff/6002/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/DEPS:2: "+third_party/zlib",
nit: You may add comment describing why do we need this include.

https://codereview.chromium.org/12213110/diff/6002/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/existing_user_controller.h (right):

https://codereview.chromium.org/12213110/diff/6002/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/existing_user_controller.h:83: virtual void
ShowWrongHWIDScreen() OVERRIDE;
nit: These are sorted.

https://codereview.chromium.org/12213110/diff/6002/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/hwid_checker.cc (right):

https://codereview.chromium.org/12213110/diff/6002/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/hwid_checker.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
nit: 2013

https://codereview.chromium.org/12213110/diff/6002/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/hwid_checker.cc:45: if
(CommandLine::ForCurrentProcess()->HasSwitch(switches::kTestType)) {
nit: Drop {}

https://codereview.chromium.org/12213110/diff/6002/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/hwid_checker.cc:48: if
(!base::chromeos::IsRunningOnChromeOS()) {
nit: Drop {}

https://codereview.chromium.org/12213110/diff/6002/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/hwid_checker.cc:54: return
stats->GetMachineStatistic("hardware_class", &hwid) &&
Please verify that GetMachineStatistic() call does not generate warning like
""Waiting to load statistics. Requested statistic"

https://codereview.chromium.org/12213110/diff/6002/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/hwid_checker.cc:54: return
stats->GetMachineStatistic("hardware_class", &hwid) &&
nit: Add LOG(ERROR) when stats->GetMachineStatistic(...) returns false.

https://codereview.chromium.org/12213110/diff/6002/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/hwid_checker.cc:55: chromeos::IsHWIDCorrect(hwid);
Please add LOG(ERROR) with the hwid value if IsHWIDCorrect(...) returns false.

https://codereview.chromium.org/12213110/diff/6002/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/hwid_checker.h (right):

https://codereview.chromium.org/12213110/diff/6002/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/hwid_checker.h:12: std::string
CalculateHWIDChecksum(const std::string& data);
nit: Comment is missing.

https://codereview.chromium.org/12213110/diff/6002/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/wizard_controller.cc (right):

https://codereview.chromium.org/12213110/diff/6002/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/wizard_controller.cc:185: ShowWrongHWIDScreen();
nit: Add {}

You could also move this above and place AdvanceToScreen(first_screen_name) call
to else block.

https://codereview.chromium.org/12213110/diff/6002/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/wizard_controller.cc:554: ShowLoginScreen();
nit: Drop {}

https://codereview.chromium.org/12213110/diff/6002/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/wrong_hwid_screen.h (right):

https://codereview.chromium.org/12213110/diff/6002/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/wrong_hwid_screen.h:17: // Representation
independent class that controls screen showing reset to users.
nit: Comments need to be updated.

https://codereview.chromium.org/12213110/diff/6002/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/wrong_hwid_screen_actor.h (right):

https://codereview.chromium.org/12213110/diff/6002/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/wrong_hwid_screen_actor.h:12: // Interface between
reset screen and its representation.
nit: Comment needs to be updated.

https://codereview.chromium.org/12213110/diff/6002/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/wrong_hwid_screen_actor.h:16: // Allows us to get
info from reset screen that we need.
nit: Same here.

https://codereview.chromium.org/12213110/diff/6002/chrome/browser/resources/c...
File chrome/browser/resources/chromeos/login/oobe.css (right):

https://codereview.chromium.org/12213110/diff/6002/chrome/browser/resources/c...
chrome/browser/resources/chromeos/login/oobe.css:507: 
nit: drop empty line

https://codereview.chromium.org/12213110/diff/6002/chrome/browser/resources/c...
chrome/browser/resources/chromeos/login/oobe.css:1458: #wrong-hwid {
nit: Please move these rules to a separate file.

https://codereview.chromium.org/12213110/diff/6002/chrome/browser/resources/c...
File chrome/browser/resources/chromeos/login/screen_wrong_hwid.js (right):

https://codereview.chromium.org/12213110/diff/6002/chrome/browser/resources/c...
chrome/browser/resources/chromeos/login/screen_wrong_hwid.js:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
nit: 2013

https://codereview.chromium.org/12213110/diff/6002/chrome/browser/ui/webui/ch...
File chrome/browser/ui/webui/chromeos/login/oobe_ui.cc (right):

https://codereview.chromium.org/12213110/diff/6002/chrome/browser/ui/webui/ch...
chrome/browser/ui/webui/chromeos/login/oobe_ui.cc:336:
screen_names_[SCREEN_CREATE_MANAGED_USER] = kScreenManagedUserCreation;
Please add new screen to screen_names_ map.

[Nikita (slow), 2013-02-11 17:09:51]

I've also executed bunch of trybots for you.

[dzhioev (left Google), 2013-02-11 23:32:56]

Most of remarks fixed.

https://codereview.chromium.org/12213110/diff/6002/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/DEPS (right):

https://codereview.chromium.org/12213110/diff/6002/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/DEPS:2: "+third_party/zlib",
On 2013/02/11 17:09:03, Nikita Kostylev wrote:
> nit: You may add comment describing why do we need this include.

Done.

https://codereview.chromium.org/12213110/diff/6002/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/existing_user_controller.h (right):

https://codereview.chromium.org/12213110/diff/6002/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/existing_user_controller.h:83: virtual void
ShowWrongHWIDScreen() OVERRIDE;
On 2013/02/11 17:09:03, Nikita Kostylev wrote:
> nit: These are sorted.

Done.

https://codereview.chromium.org/12213110/diff/6002/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/hwid_checker.cc (right):

https://codereview.chromium.org/12213110/diff/6002/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/hwid_checker.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
On 2013/02/11 17:09:03, Nikita Kostylev wrote:
> nit: 2013

Done.

https://codereview.chromium.org/12213110/diff/6002/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/hwid_checker.cc:45: if
(CommandLine::ForCurrentProcess()->HasSwitch(switches::kTestType)) {
On 2013/02/11 17:09:03, Nikita Kostylev wrote:
> nit: Drop {}

Done.

https://codereview.chromium.org/12213110/diff/6002/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/hwid_checker.cc:48: if
(!base::chromeos::IsRunningOnChromeOS()) {
On 2013/02/11 17:09:03, Nikita Kostylev wrote:
> nit: Drop {}

Done.

https://codereview.chromium.org/12213110/diff/6002/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/hwid_checker.cc:54: return
stats->GetMachineStatistic("hardware_class", &hwid) &&
On 2013/02/11 17:09:03, Nikita Kostylev wrote:
> nit: Add LOG(ERROR) when stats->GetMachineStatistic(...) returns false.

Done.

https://codereview.chromium.org/12213110/diff/6002/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/hwid_checker.cc:55: chromeos::IsHWIDCorrect(hwid);
On 2013/02/11 17:09:03, Nikita Kostylev wrote:
> Please add LOG(ERROR) with the hwid value if IsHWIDCorrect(...) returns false.

Done.

https://codereview.chromium.org/12213110/diff/6002/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/hwid_checker.h (right):

https://codereview.chromium.org/12213110/diff/6002/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/hwid_checker.h:12: std::string
CalculateHWIDChecksum(const std::string& data);
On 2013/02/11 17:09:03, Nikita Kostylev wrote:
> nit: Comment is missing.

Done.

https://codereview.chromium.org/12213110/diff/6002/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/wizard_controller.cc (right):

https://codereview.chromium.org/12213110/diff/6002/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/wizard_controller.cc:185: ShowWrongHWIDScreen();
On 2013/02/11 17:09:03, Nikita Kostylev wrote:
> nit: Add {}
> 
> You could also move this above and place AdvanceToScreen(first_screen_name)
call
> to else block.

I am not sure that it will be correct. We want to keep normal OOBE flow, so we
need some |previous_screen_|. The easiest way to do it is to show NetworkScreen
first.

https://codereview.chromium.org/12213110/diff/6002/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/wizard_controller.cc:554: ShowLoginScreen();
On 2013/02/11 17:09:03, Nikita Kostylev wrote:
> nit: Drop {}
It seems like code is incorrect here. I'll check it tomorrow.

https://codereview.chromium.org/12213110/diff/6002/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/wrong_hwid_screen.h (right):

https://codereview.chromium.org/12213110/diff/6002/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/wrong_hwid_screen.h:17: // Representation
independent class that controls screen showing reset to users.
On 2013/02/11 17:09:03, Nikita Kostylev wrote:
> nit: Comments need to be updated.

Done.

https://codereview.chromium.org/12213110/diff/6002/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/wrong_hwid_screen_actor.h (right):

https://codereview.chromium.org/12213110/diff/6002/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/wrong_hwid_screen_actor.h:12: // Interface between
reset screen and its representation.
On 2013/02/11 17:09:03, Nikita Kostylev wrote:
> nit: Comment needs to be updated.

Done.

https://codereview.chromium.org/12213110/diff/6002/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/wrong_hwid_screen_actor.h:16: // Allows us to get
info from reset screen that we need.
On 2013/02/11 17:09:03, Nikita Kostylev wrote:
> nit: Same here.

Done.

https://codereview.chromium.org/12213110/diff/6002/chrome/browser/resources/c...
File chrome/browser/resources/chromeos/login/oobe.css (right):

https://codereview.chromium.org/12213110/diff/6002/chrome/browser/resources/c...
chrome/browser/resources/chromeos/login/oobe.css:507: 
On 2013/02/11 17:09:03, Nikita Kostylev wrote:
> nit: drop empty line

Done.

https://codereview.chromium.org/12213110/diff/6002/chrome/browser/resources/c...
chrome/browser/resources/chromeos/login/oobe.css:1458: #wrong-hwid {
On 2013/02/11 17:09:03, Nikita Kostylev wrote:
> nit: Please move these rules to a separate file.

Done.

https://codereview.chromium.org/12213110/diff/6002/chrome/browser/resources/c...
File chrome/browser/resources/chromeos/login/screen_wrong_hwid.js (right):

https://codereview.chromium.org/12213110/diff/6002/chrome/browser/resources/c...
chrome/browser/resources/chromeos/login/screen_wrong_hwid.js:1: // Copyright (c)
2012 The Chromium Authors. All rights reserved.
On 2013/02/11 17:09:03, Nikita Kostylev wrote:
> nit: 2013

Done.

https://codereview.chromium.org/12213110/diff/6002/chrome/browser/ui/webui/ch...
File chrome/browser/ui/webui/chromeos/login/oobe_ui.cc (right):

https://codereview.chromium.org/12213110/diff/6002/chrome/browser/ui/webui/ch...
chrome/browser/ui/webui/chromeos/login/oobe_ui.cc:336:
screen_names_[SCREEN_CREATE_MANAGED_USER] = kScreenManagedUserCreation;
On 2013/02/11 17:09:03, Nikita Kostylev wrote:
> Please add new screen to screen_names_ map.

Done.

[Nikita (slow), 2013-02-12 07:51:50]

lgtm

https://codereview.chromium.org/12213110/diff/6002/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/wizard_controller.cc (right):

https://codereview.chromium.org/12213110/diff/6002/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/wizard_controller.cc:185: ShowWrongHWIDScreen();
On 2013/02/11 23:32:56, dzhioev wrote:
> On 2013/02/11 17:09:03, Nikita Kostylev wrote:
> > nit: Add {}
> > 
> > You could also move this above and place AdvanceToScreen(first_screen_name)
> call
> > to else block.
> 
> I am not sure that it will be correct. We want to keep normal OOBE flow, so we
> need some |previous_screen_|. The easiest way to do it is to show
NetworkScreen
> first.

I see, yes that makes sense to open first screen before HWID check then.

https://codereview.chromium.org/12213110/diff/6002/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/wizard_controller.cc:554: ShowLoginScreen();
On 2013/02/11 23:32:56, dzhioev wrote:
> On 2013/02/11 17:09:03, Nikita Kostylev wrote:
> > nit: Drop {}
> It seems like code is incorrect here. I'll check it tomorrow.

Do you mean that it has to use previous_screen_ here?

https://codereview.chromium.org/12213110/diff/12001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/hwid_checker.cc (right):

https://codereview.chromium.org/12213110/diff/12001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/hwid_checker.cc:57: LOG(ERROR) << "Machine has
malformed HWID `" << hwid << "'.";
nit: Use same style quotes.

https://codereview.chromium.org/12213110/diff/12001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/screen_observer.h (right):

https://codereview.chromium.org/12213110/diff/12001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/screen_observer.h:41: EXIT_CODES_COUNT  // not a
real code, must be the last
You have to update wizard_controller_browsertest.cc as it has compile time
assert.

[dzhioev (left Google), 2013-02-12 13:57:51]

https://codereview.chromium.org/12213110/diff/6002/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/wizard_controller.cc (right):

https://codereview.chromium.org/12213110/diff/6002/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/wizard_controller.cc:554: ShowLoginScreen();
On 2013/02/12 07:51:50, Nikita Kostylev wrote:
> On 2013/02/11 23:32:56, dzhioev wrote:
> > On 2013/02/11 17:09:03, Nikita Kostylev wrote:
> > > nit: Drop {}
> > It seems like code is incorrect here. I'll check it tomorrow.
> 
> Do you mean that it has to use previous_screen_ here?

Yes, I do. I think code shoud be same as in WizardController::OnResetCanceled.

https://codereview.chromium.org/12213110/diff/12001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/hwid_checker.cc (right):

https://codereview.chromium.org/12213110/diff/12001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/hwid_checker.cc:57: LOG(ERROR) << "Machine has
malformed HWID `" << hwid << "'.";
On 2013/02/12 07:51:50, Nikita Kostylev wrote:
> nit: Use same style quotes.

Done.

https://codereview.chromium.org/12213110/diff/12001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/screen_observer.h (right):

https://codereview.chromium.org/12213110/diff/12001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/screen_observer.h:41: EXIT_CODES_COUNT  // not a
real code, must be the last
I've written one simple test. I will write more complicated tests later.
On 2013/02/12 07:51:50, Nikita Kostylev wrote:
> You have to update wizard_controller_browsertest.cc as it has compile time
> assert.

[commit-bot: I haz the power, 2013-02-12 13:58:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dzhioev@chromium.org/12213110/12004

[commit-bot: I haz the power, 2013-02-12 17:11:04]

Change committed as 181941