EULA screen implemented....

EULA screen implemented.

BUG=crosbug.com/3178, crosbug.com/2884
TEST=out/Debug/browser_tests --gtest_filter=WizardControllerFlowTest.*


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

[glotov, 2010-07-08 14:27:36]

Hi! This is a preliminary version. I'm polishing the layout and will update
wizard controller test. However this will not change existing change much.

http://codereview.chromium.org/2920001/diff/1/2
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/2920001/diff/1/2#newcode7791
chrome/app/generated_resources.grd:7791: Optional:
Removed.

http://codereview.chromium.org/2920001/diff/1/3
File chrome/app/google_chrome_strings.grd (right):

http://codereview.chromium.org/2920001/diff/1/3#newcode357
chrome/app/google_chrome_strings.grd:357: Optional: Help make Chromium better by
automatically sending usage statistics and crash\nreports to Google
Done.

[Nikita (slow), 2010-07-08 14:38:56]

Please submit changes in grd files in a separate CL in advance.

http://codereview.chromium.org/2920001/diff/1/2
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/2920001/diff/1/2#newcode7794
chrome/app/generated_resources.grd:7794: Accept and continue »
Cool, is this an actual chevron character?
How did you added it?

http://codereview.chromium.org/2920001/diff/1/3
File chrome/app/google_chrome_strings.grd (right):

http://codereview.chromium.org/2920001/diff/1/3#newcode356
chrome/app/google_chrome_strings.grd:356: <!--We use /n here to separate lines
manually because NativeCheckboxGtk currently does not support multi-line. -->
Please place comment before <message> element.

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

http://codereview.chromium.org/2920001/diff/1/7#newcode442
chrome/browser/chromeos/login/wizard_controller.cc:442: MarkOobeCompleted();
This should be removed now.
We only mark OOBE completed once EULA is accepted.

[Nikita (slow), 2010-07-08 15:25:35]

I'll review EulaView::Init() code again once it's updated.

http://codereview.chromium.org/2920001/diff/1/4
File chrome/browser/chromeos/login/eula_view.cc (right):

http://codereview.chromium.org/2920001/diff/1/4#newcode30
chrome/browser/chromeos/login/eula_view.cc:30: const int kBorderSize = 30;
You're defining padding which is border size + margin.
Rename to kPadding or split to
kBorderSize and kMargin

http://codereview.chromium.org/2920001/diff/1/4#newcode31
chrome/browser/chromeos/login/eula_view.cc:31: const int kHorizontalSpacing =
45;
Removed unused constants.

http://codereview.chromium.org/2920001/diff/1/4#newcode80
chrome/browser/chromeos/login/eula_view.cc:80:
column_set->AddColumn(views::GridLayout::FILL, views::GridLayout::FILL, 1,
You might add
using views::GridLayout;
using views::Textfield;

and simplify several lines.

http://codereview.chromium.org/2920001/diff/1/4#newcode92
chrome/browser/chromeos/login/eula_view.cc:92: layout->StartRow(10, 0);
10 > 1

http://codereview.chromium.org/2920001/diff/1/4#newcode101
chrome/browser/chromeos/login/eula_view.cc:101: learn_more_link_ = new
views::Link();
learn_more_link_->SetController(this);

http://codereview.chromium.org/2920001/diff/1/4#newcode111
chrome/browser/chromeos/login/eula_view.cc:111: system_security_settings_link_ =
new views::Link();
system_security_settings_link_->SetController(this);

http://codereview.chromium.org/2920001/diff/1/4#newcode156
chrome/browser/chromeos/login/eula_view.cc:156: gfx::Size
EulaView::GetPreferredSize() {
I believe it's not needed.

http://codereview.chromium.org/2920001/diff/1/4#newcode164
chrome/browser/chromeos/login/eula_view.cc:164: if (sender == continue_button_)
{
TODO for cancel_button_

http://codereview.chromium.org/2920001/diff/1/4#newcode173
chrome/browser/chromeos/login/eula_view.cc:173: }  // namespace chromeos
void EulaView::LinkActivated(views::Link* source, int event_flags)

with TODOs

http://codereview.chromium.org/2920001/diff/1/5
File chrome/browser/chromeos/login/eula_view.h (right):

http://codereview.chromium.org/2920001/diff/1/5#newcode31
chrome/browser/chromeos/login/eula_view.h:31: // Initialize view layout.
controls & layout.

http://codereview.chromium.org/2920001/diff/1/5#newcode51
chrome/browser/chromeos/login/eula_view.h:51: views::Link*
system_security_settings_link_;
Might be useful to reorder controls in a way they are added at the view.

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

http://codereview.chromium.org/2920001/diff/1/7#newcode142
chrome/browser/chromeos/login/wizard_controller.cc:142: views::Accelerator
accel_image_screen_;
Please add Ctrl+Alt+E accelerator for EULA screen.

http://codereview.chromium.org/2920001/diff/1/7#newcode348
chrome/browser/chromeos/login/wizard_controller.cc:348:
SetStatusAreaVisible(true);
Status area should be hidden

http://codereview.chromium.org/2920001/diff/1/7#newcode457
chrome/browser/chromeos/login/wizard_controller.cc:457: MarkOobeCompleted();
Remove OOBE completion

http://codereview.chromium.org/2920001/diff/1/7#newcode458
chrome/browser/chromeos/login/wizard_controller.cc:458: ShowLoginScreen();
ShowEulaScreen();

[glotov, 2010-07-12 15:48:20]

http://codereview.chromium.org/2920001/diff/1/2
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/2920001/diff/1/2#newcode7791
chrome/app/generated_resources.grd:7791: Optional:
On 2010/07/08 14:27:37, glotov wrote:
> Removed.

Done.

http://codereview.chromium.org/2920001/diff/1/2#newcode7794
chrome/app/generated_resources.grd:7794: Accept and continue »
On 2010/07/08 14:38:56, Nikita Kostylev wrote:
> Cool, is this an actual chevron character?
> How did you added it?

Done.

http://codereview.chromium.org/2920001/diff/1/3
File chrome/app/google_chrome_strings.grd (right):

http://codereview.chromium.org/2920001/diff/1/3#newcode356
chrome/app/google_chrome_strings.grd:356: <!--We use /n here to separate lines
manually because NativeCheckboxGtk currently does not support multi-line. -->
On 2010/07/08 14:38:56, Nikita Kostylev wrote:
> Please place comment before <message> element.

Done.

http://codereview.chromium.org/2920001/diff/1/4
File chrome/browser/chromeos/login/eula_view.cc (right):

http://codereview.chromium.org/2920001/diff/1/4#newcode30
chrome/browser/chromeos/login/eula_view.cc:30: const int kBorderSize = 30;
On 2010/07/08 15:25:35, Nikita Kostylev wrote:
> You're defining padding which is border size + margin.
> Rename to kPadding or split to
> kBorderSize and kMargin

Done.

http://codereview.chromium.org/2920001/diff/1/4#newcode31
chrome/browser/chromeos/login/eula_view.cc:31: const int kHorizontalSpacing =
45;
On 2010/07/08 15:25:35, Nikita Kostylev wrote:
> Removed unused constants.

Done.

http://codereview.chromium.org/2920001/diff/1/4#newcode92
chrome/browser/chromeos/login/eula_view.cc:92: layout->StartRow(10, 0);
On 2010/07/08 15:25:35, Nikita Kostylev wrote:
> 10 > 1

Done.

http://codereview.chromium.org/2920001/diff/1/4#newcode101
chrome/browser/chromeos/login/eula_view.cc:101: learn_more_link_ = new
views::Link();
On 2010/07/08 15:25:35, Nikita Kostylev wrote:
> learn_more_link_->SetController(this);

Done.

http://codereview.chromium.org/2920001/diff/1/4#newcode111
chrome/browser/chromeos/login/eula_view.cc:111: system_security_settings_link_ =
new views::Link();
On 2010/07/08 15:25:35, Nikita Kostylev wrote:
> system_security_settings_link_->SetController(this);

Done.

http://codereview.chromium.org/2920001/diff/1/4#newcode156
chrome/browser/chromeos/login/eula_view.cc:156: gfx::Size
EulaView::GetPreferredSize() {
On 2010/07/08 15:25:35, Nikita Kostylev wrote:
> I believe it's not needed.

Done.

http://codereview.chromium.org/2920001/diff/1/4#newcode164
chrome/browser/chromeos/login/eula_view.cc:164: if (sender == continue_button_)
{
On 2010/07/08 15:25:35, Nikita Kostylev wrote:
> TODO for cancel_button_

Done.

http://codereview.chromium.org/2920001/diff/1/4#newcode173
chrome/browser/chromeos/login/eula_view.cc:173: }  // namespace chromeos
On 2010/07/08 15:25:35, Nikita Kostylev wrote:
> void EulaView::LinkActivated(views::Link* source, int event_flags)
> 
> with TODOs

Done.

http://codereview.chromium.org/2920001/diff/1/5
File chrome/browser/chromeos/login/eula_view.h (right):

http://codereview.chromium.org/2920001/diff/1/5#newcode31
chrome/browser/chromeos/login/eula_view.h:31: // Initialize view layout.
On 2010/07/08 15:25:35, Nikita Kostylev wrote:
> controls & layout.

Done.

http://codereview.chromium.org/2920001/diff/1/5#newcode51
chrome/browser/chromeos/login/eula_view.h:51: views::Link*
system_security_settings_link_;
On 2010/07/08 15:25:35, Nikita Kostylev wrote:
> Might be useful to reorder controls in a way they are added at the view.

Done.

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

http://codereview.chromium.org/2920001/diff/1/7#newcode142
chrome/browser/chromeos/login/wizard_controller.cc:142: views::Accelerator
accel_image_screen_;
On 2010/07/08 15:25:35, Nikita Kostylev wrote:
> Please add Ctrl+Alt+E accelerator for EULA screen.

Done.

http://codereview.chromium.org/2920001/diff/1/7#newcode348
chrome/browser/chromeos/login/wizard_controller.cc:348:
SetStatusAreaVisible(true);
On 2010/07/08 15:25:35, Nikita Kostylev wrote:
> Status area should be hidden

Done.

http://codereview.chromium.org/2920001/diff/1/7#newcode442
chrome/browser/chromeos/login/wizard_controller.cc:442: MarkOobeCompleted();
On 2010/07/08 14:38:56, Nikita Kostylev wrote:
> This should be removed now.
> We only mark OOBE completed once EULA is accepted.

Done.

http://codereview.chromium.org/2920001/diff/1/7#newcode457
chrome/browser/chromeos/login/wizard_controller.cc:457: MarkOobeCompleted();
On 2010/07/08 15:25:35, Nikita Kostylev wrote:
> Remove OOBE completion

Done.

http://codereview.chromium.org/2920001/diff/1/7#newcode458
chrome/browser/chromeos/login/wizard_controller.cc:458: ShowLoginScreen();
On 2010/07/08 15:25:35, Nikita Kostylev wrote:
> ShowEulaScreen();

Done.

[Nikita (slow), 2010-07-12 16:53:26]

http://codereview.chromium.org/2920001/diff/1/4
File chrome/browser/chromeos/login/eula_view.cc (right):

http://codereview.chromium.org/2920001/diff/1/4#newcode80
chrome/browser/chromeos/login/eula_view.cc:80:
column_set->AddColumn(views::GridLayout::FILL, views::GridLayout::FILL, 1,
On 2010/07/08 15:25:35, Nikita Kostylev wrote:
> You might add
> using views::GridLayout;
> using views::Textfield;
> 
> and simplify several lines.

Please add uses - that will simplify code.
views::GridLayout is used multiply times.

http://codereview.chromium.org/2920001/diff/13001/14001#newcode30
chrome/browser/chromeos/login/eula_view.cc:30: const int kBorderSize = 30;
kPadding is still used as a single padding for whole layout (top, bottom, left,
right)

You should define 
int padding = kBorderSize + kMargin;
and use it instead of kPadding.

http://codereview.chromium.org/2920001/diff/13001/14001#newcode32
chrome/browser/chromeos/login/eula_view.cc:32: const int kHorizontalIntraSpacing
= 15;
Comments?

http://codereview.chromium.org/2920001/diff/13001/14001#newcode109
chrome/browser/chromeos/login/eula_view.cc:109: 
#include "views/standard_layout.h"

kRelatedControlHorizontalSpacing

http://codereview.chromium.org/2920001/diff/13001/14001#newcode141
chrome/browser/chromeos/login/eula_view.cc:141:
l10n_util::GetString(IDS_EULS_SYSTEM_SECURITY_SETTINGS_LINK));
kRelatedControlVerticalSpacing or
kRelatedControlSmallVerticalSpacing ?

[glotov, 2010-07-13 13:56:39]

http://codereview.chromium.org/2920001/diff/13001/14001
File chrome/browser/chromeos/login/eula_view.cc (right):

http://codereview.chromium.org/2920001/diff/13001/14001#newcode30
chrome/browser/chromeos/login/eula_view.cc:30: const int kBorderSize = 10;
On 2010/07/12 16:53:27, Nikita Kostylev wrote:
> kPadding is still used as a single padding for whole layout (top, bottom,
left,
> right)
> 
> You should define 
> int padding = kBorderSize + kMargin;
> and use it instead of kPadding.

Done.

http://codereview.chromium.org/2920001/diff/13001/14001#newcode32
chrome/browser/chromeos/login/eula_view.cc:32: const int kIntraPadding = 10;
On 2010/07/12 16:53:27, Nikita Kostylev wrote:
> Comments?

Done.

http://codereview.chromium.org/2920001/diff/13001/14001#newcode109
chrome/browser/chromeos/login/eula_view.cc:109: column_set->AddPaddingColumn(0,
kIntraPadding);
On 2010/07/12 16:53:27, Nikita Kostylev wrote:
> #include "views/standard_layout.h"
> 
> kRelatedControlHorizontalSpacing

Done.

http://codereview.chromium.org/2920001/diff/13001/14001#newcode141
chrome/browser/chromeos/login/eula_view.cc:141: layout->AddPaddingRow(0,
kIntraPadding);
On 2010/07/12 16:53:27, Nikita Kostylev wrote:
> kRelatedControlVerticalSpacing or
> kRelatedControlSmallVerticalSpacing ?

Done.

[Nikita (slow), 2010-07-13 14:23:04]

LGTM