Added "Customize Sync" dialog, removed "Merge and Sync" step.

The "Customize Sync" dialog will let users select to sync or not sync each data type (bookmarks, preferences, etc).

The Customize Sync dialog appears if you click a button on the gaia login or the Options->Personal Stuff tab.  This button only appears if you've set the --enable-sync-preferences or --enable-sync-autofill command-line flag.
On the Gaia login, the Customize Sync button grays out when you click 'sign in'.  If the "customize sync" dialog is open, it closes when you click "cancel" on the Gaia login, and it accepts when you log in to Gaia.

Removed "Merge and Sync" from the login sequence.  Also deleted the 'merge_allowed' parameter from DataTypeController and its subclasses.

Fixed strings so they all refer to "Google Chrome/Chromium sync" instead of "Bookmark sync".

BUG=34209, 27259
TEST=none

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

[Dan Tasse, 2010-03-24 00:28:03]

'lo Steve, Fred, Tim!  Here it is, big UI CL #1.  To help break up the review:

Steve- chrome/browser/sync/glue/* and chrome/browser/sync/syncable/model_type.h
Fred- chrome/browser/sync/resources/* and chrome/browser/sync/*
Tim- chrome/app/..., chrome/browser/browser_resources.grd,
chrome/browser/views/options/*, and chrome/chrome_browser.gypi.

Of course, feel free to reassign.  Thank you much!
Dan

[skrul, 2010-03-24 21:47:53]

LGTM with a few nits

http://codereview.chromium.org/1134002/diff/11001/12004
File chrome/browser/sync/glue/autofill_data_type_controller.cc (right):

http://codereview.chromium.org/1134002/diff/11001/12004#newcode107
chrome/browser/sync/glue/autofill_data_type_controller.cc:107: bool
chrome_has_nodes = false;
chrome_has_nodes is no longer used, plz remove.

http://codereview.chromium.org/1134002/diff/11001/12004#newcode123
chrome/browser/sync/glue/autofill_data_type_controller.cc:123:
StartFailed(ASSOCIATION_FAILED);
good catch, this was actually wrong.

http://codereview.chromium.org/1134002/diff/11001/12011
File chrome/browser/sync/glue/data_type_manager_impl.cc (right):

http://codereview.chromium.org/1134002/diff/11001/12011#newcode14
chrome/browser/sync/glue/data_type_manager_impl.cc:14: #include
"chrome/browser/sync/syncable/model_type.h"
why add this?  it is included via data_type_manager.h

http://codereview.chromium.org/1134002/diff/11001/12015
File chrome/browser/sync/glue/preference_data_type_controller_unittest.cc
(right):

http://codereview.chromium.org/1134002/diff/11001/12015#newcode102
chrome/browser/sync/glue/preference_data_type_controller_unittest.cc:102:
TEST_F(PreferenceDataTypeControllerTest, StartMergeAllowed) {
Maybe rename to "StartOK" like you did elsewhere?

http://codereview.chromium.org/1134002/diff/11001/12016
File chrome/browser/sync/glue/typed_url_data_type_controller.cc (right):

http://codereview.chromium.org/1134002/diff/11001/12016#newcode128
chrome/browser/sync/glue/typed_url_data_type_controller.cc:128: bool
chrome_has_nodes = false;
no longer used, plz remove.

http://codereview.chromium.org/1134002/diff/11001/12016#newcode144
chrome/browser/sync/glue/typed_url_data_type_controller.cc:144:
StartFailed(ASSOCIATION_FAILED);
again, thx for finding this.

[Dan Tasse, 2010-03-24 21:59:05]

Thanks!

http://codereview.chromium.org/1134002/diff/11001/12004
File chrome/browser/sync/glue/autofill_data_type_controller.cc (right):

http://codereview.chromium.org/1134002/diff/11001/12004#newcode107
chrome/browser/sync/glue/autofill_data_type_controller.cc:107: bool
chrome_has_nodes = false;
On 2010/03/24 21:47:53, skrul wrote:
> chrome_has_nodes is no longer used, plz remove.

Done.

http://codereview.chromium.org/1134002/diff/11001/12004#newcode123
chrome/browser/sync/glue/autofill_data_type_controller.cc:123:
StartFailed(ASSOCIATION_FAILED);
On 2010/03/24 21:47:53, skrul wrote:
> good catch, this was actually wrong.
> 

Done.

http://codereview.chromium.org/1134002/diff/11001/12011
File chrome/browser/sync/glue/data_type_manager_impl.cc (right):

http://codereview.chromium.org/1134002/diff/11001/12011#newcode14
chrome/browser/sync/glue/data_type_manager_impl.cc:14: #include
"chrome/browser/sync/syncable/model_type.h"
On 2010/03/24 21:47:53, skrul wrote:
> why add this?  it is included via data_type_manager.h

Right, it was probably a mistake.

http://codereview.chromium.org/1134002/diff/11001/12015
File chrome/browser/sync/glue/preference_data_type_controller_unittest.cc
(right):

http://codereview.chromium.org/1134002/diff/11001/12015#newcode102
chrome/browser/sync/glue/preference_data_type_controller_unittest.cc:102:
TEST_F(PreferenceDataTypeControllerTest, StartMergeAllowed) {
On 2010/03/24 21:47:53, skrul wrote:
> Maybe rename to "StartOK" like you did elsewhere?

Done.

http://codereview.chromium.org/1134002/diff/11001/12016
File chrome/browser/sync/glue/typed_url_data_type_controller.cc (right):

http://codereview.chromium.org/1134002/diff/11001/12016#newcode128
chrome/browser/sync/glue/typed_url_data_type_controller.cc:128: bool
chrome_has_nodes = false;
On 2010/03/24 21:47:53, skrul wrote:
> no longer used, plz remove.

Done.

http://codereview.chromium.org/1134002/diff/11001/12016#newcode144
chrome/browser/sync/glue/typed_url_data_type_controller.cc:144:
StartFailed(ASSOCIATION_FAILED);
On 2010/03/24 21:47:53, skrul wrote:
> again, thx for finding this.

Done.

[akalin, 2010-03-24 22:11:10]

http://codereview.chromium.org/1134002/diff/11001/12018
File chrome/browser/sync/profile_sync_service.cc (right):

http://codereview.chromium.org/1134002/diff/11001/12018#newcode417
chrome/browser/sync/profile_sync_service.cc:417: if
(bootstrap_sync_authentication_)
keep braces

http://codereview.chromium.org/1134002/diff/11001/12030
File chrome/browser/views/options/content_page_view.cc (left):

http://codereview.chromium.org/1134002/diff/11001/12030#oldcode65
chrome/browser/views/options/content_page_view.cc:65: sync_status_label_(NULL),
what happened to this line?

[Dan Tasse, 2010-03-24 22:32:36]

Thanks #2!

http://codereview.chromium.org/1134002/diff/11001/12018
File chrome/browser/sync/profile_sync_service.cc (right):

http://codereview.chromium.org/1134002/diff/11001/12018#newcode417
chrome/browser/sync/profile_sync_service.cc:417: if
(bootstrap_sync_authentication_)
On 2010/03/24 22:11:11, akalin wrote:
> keep braces

Done.

http://codereview.chromium.org/1134002/diff/11001/12030
File chrome/browser/views/options/content_page_view.cc (left):

http://codereview.chromium.org/1134002/diff/11001/12030#oldcode65
chrome/browser/views/options/content_page_view.cc:65: sync_status_label_(NULL),
On 2010/03/24 22:11:11, akalin wrote:
> what happened to this line?

Good question.  Apparently stuff still works without it.  What does it do?  It
looks to me like it's initializing sync_status_label_ to NULL, and we just
always make sure to set sync_status_label_ before we use it so it hasn't caused
any problems.  Is that right?

At any rate, I've added the line back in.

http://codereview.chromium.org/1134002/diff/11001/12030
File chrome/browser/views/options/content_page_view.cc (right):

http://codereview.chromium.org/1134002/diff/11001/12030#newcode161
chrome/browser/views/options/content_page_view.cc:161: // TODO(dantasse) should
we have a singleton that we just show here instead?
Note to myself: yes, we should just show the singleton here instead.  Fixed.

[tfarina (gmail-do not use), 2010-03-26 21:20:49]

http://codereview.chromium.org/1134002/diff/22001/23033
File chrome/browser/views/options/customize_sync_window_view.cc (right):

http://codereview.chromium.org/1134002/diff/22001/23033#newcode1
chrome/browser/views/options/customize_sync_window_view.cc:1: // Copyright (c)
2009 The Chromium Authors. All rights reserved.
nit: 2010

http://codereview.chromium.org/1134002/diff/22001/23033#newcode30
chrome/browser/views/options/customize_sync_window_view.cc:30: bool
configure_on_accept_;
nit: why global variables? I think that belongs to the private section of the
class.

http://codereview.chromium.org/1134002/diff/22001/23033#newcode40
chrome/browser/views/options/customize_sync_window_view.cc:40: Profile* profile,
nit: align with gfx::NativeWindow

http://codereview.chromium.org/1134002/diff/22001/23033#newcode58
chrome/browser/views/options/customize_sync_window_view.cc:58: // Ideally this
would do the same as when you click "ok".
nit: can you move this comment to the header file?

http://codereview.chromium.org/1134002/diff/22001/23033#newcode67
chrome/browser/views/options/customize_sync_window_view.cc:67: // Ideally this
would do the same as when you click "cancel".
nit: can you move this comment to the header file?

http://codereview.chromium.org/1134002/diff/22001/23033#newcode84
chrome/browser/views/options/customize_sync_window_view.cc:84:
l10n_util::GetString(IDS_CUSTOMIZE_SYNC_DESCRIPTION));
nit: indent 4 spaces.

http://codereview.chromium.org/1134002/diff/22001/23033#newcode87
chrome/browser/views/options/customize_sync_window_view.cc:87:
rb.GetFont(ResourceBundle::BaseFont).DeriveFont(0, gfx::Font::BOLD);
nit: indent 4 spaces.

http://codereview.chromium.org/1134002/diff/22001/23033#newcode96
chrome/browser/views/options/customize_sync_window_view.cc:96:
!configure_on_accept_ ||
nit: indent 4 spaces.

http://codereview.chromium.org/1134002/diff/22001/23033#newcode100
chrome/browser/views/options/customize_sync_window_view.cc:100:
l10n_util::GetString(IDS_SYNC_DATATYPE_BOOKMARKS), bookmarks_checked);
nit: same here.

http://codereview.chromium.org/1134002/diff/22001/23034
File chrome/browser/views/options/customize_sync_window_view.h (right):

http://codereview.chromium.org/1134002/diff/22001/23034#newcode1
chrome/browser/views/options/customize_sync_window_view.h:1: // Copyright (c)
2009 The Chromium Authors. All rights reserved.
nit: 2010

http://codereview.chromium.org/1134002/diff/22001/23034#newcode13
chrome/browser/views/options/customize_sync_window_view.h:13: class Checkbox;
nit: no need to have spaces here. This is aligned with namespace.

http://codereview.chromium.org/1134002/diff/22001/23034#newcode25
chrome/browser/views/options/customize_sync_window_view.h:25: void Init();
nit: I think you could move this function to the private section. Or are you
going to call it from outside?

http://codereview.chromium.org/1134002/diff/22001/23034#newcode27
chrome/browser/views/options/customize_sync_window_view.h:27: // Show the
CustomizeSyncWindowView for the given profile. parent_window is
nit: |parent_window|

http://codereview.chromium.org/1134002/diff/22001/23034#newcode29
chrome/browser/views/options/customize_sync_window_view.h:29: // TODO(dantasse)
when we make this window modal, parent_window will not be
nit: |parent_window|

http://codereview.chromium.org/1134002/diff/22001/23034#newcode32
chrome/browser/views/options/customize_sync_window_view.h:32: bool
configure_on_accept);
nit: align with gfx::NativeWindow? or 4 spaces.

http://codereview.chromium.org/1134002/diff/22001/23034#newcode43
chrome/browser/views/options/customize_sync_window_view.h:43: views::View*
child);
nit: align with |is_add|.

http://codereview.chromium.org/1134002/diff/22001/23034#newcode59
chrome/browser/views/options/customize_sync_window_view.h:59: 
nit: remove blank line.

[tfarina (gmail-do not use), 2010-03-26 21:24:52]

http://codereview.chromium.org/1134002/diff/22001/23031
File chrome/browser/views/options/content_page_view.cc (right):

http://codereview.chromium.org/1134002/diff/22001/23031#newcode161
chrome/browser/views/options/content_page_view.cc:161: true);
nit: true doesn't fit into the above line?

http://codereview.chromium.org/1134002/diff/22001/23031#newcode468
chrome/browser/views/options/content_page_view.cc:468: //TODO(dantasse) Remove
this big "if" when multi-datatype sync is live.
nit: // TODO (dantasse):

http://codereview.chromium.org/1134002/diff/22001/23031#newcode472
chrome/browser/views/options/content_page_view.cc:472:
switches::kEnableSyncAutofill)) {
nit: indent 4 spaces.

[Dan Tasse, 2010-03-26 21:48:34]

http://codereview.chromium.org/1134002/diff/22001/23031
File chrome/browser/views/options/content_page_view.cc (right):

http://codereview.chromium.org/1134002/diff/22001/23031#newcode161
chrome/browser/views/options/content_page_view.cc:161: true);
On 2010/03/26 21:24:52, tfarina wrote:
> nit: true doesn't fit into the above line?

Nope.

http://codereview.chromium.org/1134002/diff/22001/23031#newcode468
chrome/browser/views/options/content_page_view.cc:468: //TODO(dantasse) Remove
this big "if" when multi-datatype sync is live.
On 2010/03/26 21:24:52, tfarina wrote:
> nit: // TODO (dantasse):

Done.

http://codereview.chromium.org/1134002/diff/22001/23031#newcode472
chrome/browser/views/options/content_page_view.cc:472:
switches::kEnableSyncAutofill)) {
On 2010/03/26 21:24:52, tfarina wrote:
> nit: indent 4 spaces.

Done.

http://codereview.chromium.org/1134002/diff/22001/23033
File chrome/browser/views/options/customize_sync_window_view.cc (right):

http://codereview.chromium.org/1134002/diff/22001/23033#newcode1
chrome/browser/views/options/customize_sync_window_view.cc:1: // Copyright (c)
2009 The Chromium Authors. All rights reserved.
On 2010/03/26 21:20:50, tfarina wrote:
> nit: 2010

Done.

http://codereview.chromium.org/1134002/diff/22001/23033#newcode30
chrome/browser/views/options/customize_sync_window_view.cc:30: bool
configure_on_accept_;
On 2010/03/26 21:20:50, tfarina wrote:
> nit: why global variables? I think that belongs to the private section of the
> class.

Done.

http://codereview.chromium.org/1134002/diff/22001/23033#newcode40
chrome/browser/views/options/customize_sync_window_view.cc:40: Profile* profile,
On 2010/03/26 21:20:50, tfarina wrote:
> nit: align with gfx::NativeWindow

Done.

http://codereview.chromium.org/1134002/diff/22001/23033#newcode58
chrome/browser/views/options/customize_sync_window_view.cc:58: // Ideally this
would do the same as when you click "ok".
On 2010/03/26 21:20:50, tfarina wrote:
> nit: can you move this comment to the header file?

Done.

http://codereview.chromium.org/1134002/diff/22001/23033#newcode67
chrome/browser/views/options/customize_sync_window_view.cc:67: // Ideally this
would do the same as when you click "cancel".
On 2010/03/26 21:20:50, tfarina wrote:
> nit: can you move this comment to the header file?

Done.

http://codereview.chromium.org/1134002/diff/22001/23033#newcode84
chrome/browser/views/options/customize_sync_window_view.cc:84:
l10n_util::GetString(IDS_CUSTOMIZE_SYNC_DESCRIPTION));
On 2010/03/26 21:20:50, tfarina wrote:
> nit: indent 4 spaces.

Done.

http://codereview.chromium.org/1134002/diff/22001/23033#newcode87
chrome/browser/views/options/customize_sync_window_view.cc:87:
rb.GetFont(ResourceBundle::BaseFont).DeriveFont(0, gfx::Font::BOLD);
On 2010/03/26 21:20:50, tfarina wrote:
> nit: indent 4 spaces.

Done.

http://codereview.chromium.org/1134002/diff/22001/23033#newcode96
chrome/browser/views/options/customize_sync_window_view.cc:96:
!configure_on_accept_ ||
On 2010/03/26 21:20:50, tfarina wrote:
> nit: indent 4 spaces.

Done.

http://codereview.chromium.org/1134002/diff/22001/23033#newcode100
chrome/browser/views/options/customize_sync_window_view.cc:100:
l10n_util::GetString(IDS_SYNC_DATATYPE_BOOKMARKS), bookmarks_checked);
On 2010/03/26 21:20:50, tfarina wrote:
> nit: same here.

Done.

http://codereview.chromium.org/1134002/diff/22001/23034
File chrome/browser/views/options/customize_sync_window_view.h (right):

http://codereview.chromium.org/1134002/diff/22001/23034#newcode1
chrome/browser/views/options/customize_sync_window_view.h:1: // Copyright (c)
2009 The Chromium Authors. All rights reserved.
On 2010/03/26 21:20:50, tfarina wrote:
> nit: 2010

Done.

http://codereview.chromium.org/1134002/diff/22001/23034#newcode13
chrome/browser/views/options/customize_sync_window_view.h:13: class Checkbox;
On 2010/03/26 21:20:50, tfarina wrote:
> nit: no need to have spaces here. This is aligned with namespace.

Done.

http://codereview.chromium.org/1134002/diff/22001/23034#newcode25
chrome/browser/views/options/customize_sync_window_view.h:25: void Init();
On 2010/03/26 21:20:50, tfarina wrote:
> nit: I think you could move this function to the private section. Or are you
> going to call it from outside?

Done.

http://codereview.chromium.org/1134002/diff/22001/23034#newcode27
chrome/browser/views/options/customize_sync_window_view.h:27: // Show the
CustomizeSyncWindowView for the given profile. parent_window is
On 2010/03/26 21:20:50, tfarina wrote:
> nit: |parent_window|

Done.

http://codereview.chromium.org/1134002/diff/22001/23034#newcode29
chrome/browser/views/options/customize_sync_window_view.h:29: // TODO(dantasse)
when we make this window modal, parent_window will not be
On 2010/03/26 21:20:50, tfarina wrote:
> nit: |parent_window|

Done.

http://codereview.chromium.org/1134002/diff/22001/23034#newcode32
chrome/browser/views/options/customize_sync_window_view.h:32: bool
configure_on_accept);
On 2010/03/26 21:20:50, tfarina wrote:
> nit: align with gfx::NativeWindow? or 4 spaces.

Done.

http://codereview.chromium.org/1134002/diff/22001/23034#newcode43
chrome/browser/views/options/customize_sync_window_view.h:43: views::View*
child);
On 2010/03/26 21:20:50, tfarina wrote:
> nit: align with |is_add|.

Done.

http://codereview.chromium.org/1134002/diff/22001/23034#newcode59
chrome/browser/views/options/customize_sync_window_view.h:59: 
On 2010/03/26 21:20:50, tfarina wrote:
> nit: remove blank line.

Done.

[tim (not reviewing), 2010-03-26 22:45:09]

One question, few nits, then LGTM

http://codereview.chromium.org/1134002/diff/22001/23002
File chrome/app/resources/locale_settings.grd (right):

http://codereview.chromium.org/1134002/diff/22001/23002#newcode315
chrome/app/resources/locale_settings.grd:315: 10
I think we'll need to add these to the .xtbs, but follow up change is okay.

http://codereview.chromium.org/1134002/diff/22001/23031
File chrome/browser/views/options/content_page_view.cc (right):

http://codereview.chromium.org/1134002/diff/22001/23031#newcode140
chrome/browser/views/options/content_page_view.cc:140:
GetWindow()->GetNativeWindow(),
this block of parameters needs to be indented 2 more spaces.

http://codereview.chromium.org/1134002/diff/22001/23031#newcode161
chrome/browser/views/options/content_page_view.cc:161: true);
nit - 2 more spaces indent, or align under GetWindow.

http://codereview.chromium.org/1134002/diff/31001/32033
File chrome/browser/views/options/customize_sync_window_view.cc (right):

http://codereview.chromium.org/1134002/diff/31001/32033#newcode27
chrome/browser/views/options/customize_sync_window_view.cc:27: :
profile_(profile) {
you should init the other members (e.g. configure_on_accept_) if at all
possible, will help avoid UMR reports from valgrind.

http://codereview.chromium.org/1134002/diff/31001/32033#newcode201
chrome/browser/views/options/customize_sync_window_view.cc:201:
!configure_on_accept_ ||
the bool computation should be indented 4 spaces from the start of the previous
line 'bool bookmarks....'

http://codereview.chromium.org/1134002/diff/31001/32033#newcode219
chrome/browser/views/options/customize_sync_window_view.cc:219:
!configure_on_accept_ ||
same as above

http://codereview.chromium.org/1134002/diff/31001/32034
File chrome/browser/views/options/customize_sync_window_view.h (right):

http://codereview.chromium.org/1134002/diff/31001/32034#newcode33
chrome/browser/views/options/customize_sync_window_view.h:33: // if it exists.
simulate? What does this mean? where are these used?

[Dan Tasse, 2010-03-30 19:50:28]

http://codereview.chromium.org/1134002/diff/22001/23002
File chrome/app/resources/locale_settings.grd (right):

http://codereview.chromium.org/1134002/diff/22001/23002#newcode315
chrome/app/resources/locale_settings.grd:315: 10
On 2010/03/26 22:45:10, timsteele wrote:
> I think we'll need to add these to the .xtbs, but follow up change is okay.

Will do.  Thanks for letting me know.

http://codereview.chromium.org/1134002/diff/22001/23031
File chrome/browser/views/options/content_page_view.cc (right):

http://codereview.chromium.org/1134002/diff/22001/23031#newcode140
chrome/browser/views/options/content_page_view.cc:140:
GetWindow()->GetNativeWindow(),
On 2010/03/26 22:45:10, timsteele wrote:
> this block of parameters needs to be indented 2 more spaces.

Done.

http://codereview.chromium.org/1134002/diff/22001/23031#newcode161
chrome/browser/views/options/content_page_view.cc:161: true);
On 2010/03/26 22:45:10, timsteele wrote:
> nit - 2 more spaces indent, or align under GetWindow.

Done.

http://codereview.chromium.org/1134002/diff/31001/32033
File chrome/browser/views/options/customize_sync_window_view.cc (right):

http://codereview.chromium.org/1134002/diff/31001/32033#newcode27
chrome/browser/views/options/customize_sync_window_view.cc:27: :
profile_(profile) {
On 2010/03/26 22:45:10, timsteele wrote:
> you should init the other members (e.g. configure_on_accept_) if at all
> possible, will help avoid UMR reports from valgrind.

Ok, I'm init-ing configure_on_accept_.  But not the other UI elements, because
they all get initialized in init().  That seems the most consistent with other
Views I've looked at.  Sound good?

http://codereview.chromium.org/1134002/diff/31001/32033#newcode201
chrome/browser/views/options/customize_sync_window_view.cc:201:
!configure_on_accept_ ||
On 2010/03/26 22:45:10, timsteele wrote:
> the bool computation should be indented 4 spaces from the start of the
previous
> line 'bool bookmarks....'

Done.

http://codereview.chromium.org/1134002/diff/31001/32033#newcode219
chrome/browser/views/options/customize_sync_window_view.cc:219:
!configure_on_accept_ ||
On 2010/03/26 22:45:10, timsteele wrote:
> same as above

Done.

http://codereview.chromium.org/1134002/diff/31001/32034
File chrome/browser/views/options/customize_sync_window_view.h (right):

http://codereview.chromium.org/1134002/diff/31001/32034#newcode33
chrome/browser/views/options/customize_sync_window_view.h:33: // if it exists.
On 2010/03/26 22:45:10, timsteele wrote:
> simulate? What does this mean? where are these used?

I mean "do the same thing as it would if the user clicked OK."  Maybe I should
abstract this in the future a little bit, but I didn't know what to call it
("Accept()", for example, is already taken).

[tim (not reviewing), 2010-03-30 19:55:34]

LGTM

On Tue, Mar 30, 2010 at 12:50 PM, <[email protected]> wrote:

>
> http://codereview.chromium.org/1134002/diff/22001/23002
> File chrome/app/resources/locale_settings.grd (right):
>
> http://codereview.chromium.org/1134002/diff/22001/23002#newcode315
> chrome/app/resources/locale_settings.grd:315: 10
> On 2010/03/26 22:45:10, timsteele wrote:
>
>> I think we'll need to add these to the .xtbs, but follow up change is
>>
> okay.
>
> Will do.  Thanks for letting me know.
>
>
> http://codereview.chromium.org/1134002/diff/22001/23031
> File chrome/browser/views/options/content_page_view.cc (right):
>
> http://codereview.chromium.org/1134002/diff/22001/23031#newcode140
> chrome/browser/views/options/content_page_view.cc:140:
> GetWindow()->GetNativeWindow(),
> On 2010/03/26 22:45:10, timsteele wrote:
>
>> this block of parameters needs to be indented 2 more spaces.
>>
>
> Done.
>
>
> http://codereview.chromium.org/1134002/diff/22001/23031#newcode161
> chrome/browser/views/options/content_page_view.cc:161: true);
> On 2010/03/26 22:45:10, timsteele wrote:
>
>> nit - 2 more spaces indent, or align under GetWindow.
>>
>
> Done.
>
>
> http://codereview.chromium.org/1134002/diff/31001/32033
> File chrome/browser/views/options/customize_sync_window_view.cc (right):
>
> http://codereview.chromium.org/1134002/diff/31001/32033#newcode27
> chrome/browser/views/options/customize_sync_window_view.cc:27: :
> profile_(profile) {
> On 2010/03/26 22:45:10, timsteele wrote:
>
>> you should init the other members (e.g. configure_on_accept_) if at
>>
> all
>
>> possible, will help avoid UMR reports from valgrind.
>>
>
> Ok, I'm init-ing configure_on_accept_.  But not the other UI elements,
> because they all get initialized in init().  That seems the most
> consistent with other Views I've looked at.  Sound good?
>
>
> http://codereview.chromium.org/1134002/diff/31001/32033#newcode201
> chrome/browser/views/options/customize_sync_window_view.cc:201:
> !configure_on_accept_ ||
> On 2010/03/26 22:45:10, timsteele wrote:
>
>> the bool computation should be indented 4 spaces from the start of the
>>
> previous
>
>> line 'bool bookmarks....'
>>
>
> Done.
>
>
> http://codereview.chromium.org/1134002/diff/31001/32033#newcode219
> chrome/browser/views/options/customize_sync_window_view.cc:219:
> !configure_on_accept_ ||
> On 2010/03/26 22:45:10, timsteele wrote:
>
>> same as above
>>
>
> Done.
>
>
> http://codereview.chromium.org/1134002/diff/31001/32034
> File chrome/browser/views/options/customize_sync_window_view.h (right):
>
> http://codereview.chromium.org/1134002/diff/31001/32034#newcode33
> chrome/browser/views/options/customize_sync_window_view.h:33: // if it
> exists.
> On 2010/03/26 22:45:10, timsteele wrote:
>
>> simulate? What does this mean? where are these used?
>>
>
> I mean "do the same thing as it would if the user clicked OK."  Maybe I
> should abstract this in the future a little bit, but I didn't know what
> to call it ("Accept()", for example, is already taken).
>
>
> http://codereview.chromium.org/1134002
>

[tfarina (gmail-do not use), 2010-03-30 19:57:19]

http://codereview.chromium.org/1134002/diff/31001/32033
File chrome/browser/views/options/customize_sync_window_view.cc (right):

http://codereview.chromium.org/1134002/diff/31001/32033#newcode27
chrome/browser/views/options/customize_sync_window_view.cc:27: :
profile_(profile) {
Maybe. But I think you could take look at other examples under
chrome/browser/views. So I took three examples:

http://git.chromium.org/cgi-bin/gitweb.cgi?p=chromium.git;a=blob;f=chrome/bro...
http://git.chromium.org/cgi-bin/gitweb.cgi?p=chromium.git;a=blob;f=chrome/bro...
http://git.chromium.org/cgi-bin/gitweb.cgi?p=chromium.git;a=blob;f=chrome/bro...

[arv (Not doing code reviews), 2010-03-30 20:21:53]

drive by review

The HTML, JS and CSS could use some fixes. It has a lot of style issues but also
some bugs.

http://codereview.chromium.org/1134002/diff/38001/39023
File chrome/browser/sync/resources/gaia_login.html (right):

http://codereview.chromium.org/1134002/diff/38001/39023#newcode2
chrome/browser/sync/resources/gaia_login.html:2: <style type="text/css"><!--
Remove HTML comment

http://codereview.chromium.org/1134002/diff/38001/39023#newcode4
chrome/browser/sync/resources/gaia_login.html:4: body { bgcolor:"#ffffff" }
Invalid style property. Did you mean background-color>

http://codereview.chromium.org/1134002/diff/38001/39023#newcode4
chrome/browser/sync/resources/gaia_login.html:4: body { bgcolor:"#ffffff" }
Invalid value. Did you mean #fff or #ffffff ?

http://codereview.chromium.org/1134002/diff/38001/39023#newcode493
chrome/browser/sync/resources/gaia_login.html:493: i18n-values="value:customize"
onClick="showCustomize();"
s/onClick/onclick/

http://codereview.chromium.org/1134002/diff/38001/39023#newcode494
chrome/browser/sync/resources/gaia_login.html:494:
style="width:150;visibility:hidden" disabled="true"/>
<input ... >

no />

http://codereview.chromium.org/1134002/diff/38001/39025
File chrome/browser/sync/resources/setup_flow.html (right):

http://codereview.chromium.org/1134002/diff/38001/39025#newcode1
chrome/browser/sync/resources/setup_flow.html:1: <HTML id='t'>
Missing doctype

http://codereview.chromium.org/1134002/diff/38001/39025#newcode1
chrome/browser/sync/resources/setup_flow.html:1: <HTML id='t'>
Use lowercase tag names

http://codereview.chromium.org/1134002/diff/38001/39025#newcode6
chrome/browser/sync/resources/setup_flow.html:6:
document.getElementById("login").style.display = "none";
use single quotes in js

[Dan Tasse, 2010-03-30 21:54:20]

Thanks, Erik.  I've fixed most of the changes.  I'm not changing the doctype or
the <!-- --> comments right now, because those might change the way things look;
I'll do it in a later CL.

http://codereview.chromium.org/1134002/diff/38001/39023
File chrome/browser/sync/resources/gaia_login.html (right):

http://codereview.chromium.org/1134002/diff/38001/39023#newcode4
chrome/browser/sync/resources/gaia_login.html:4: body { bgcolor:"#ffffff" }
On 2010/03/30 20:21:53, arv wrote:
> Invalid style property. Did you mean background-color>

Done.

http://codereview.chromium.org/1134002/diff/38001/39023#newcode4
chrome/browser/sync/resources/gaia_login.html:4: body { bgcolor:"#ffffff" }
On 2010/03/30 20:21:53, arv wrote:
> Invalid value. Did you mean #fff or #ffffff ?

Done.

http://codereview.chromium.org/1134002/diff/38001/39023#newcode493
chrome/browser/sync/resources/gaia_login.html:493: i18n-values="value:customize"
onClick="showCustomize();"
On 2010/03/30 20:21:53, arv wrote:
> s/onClick/onclick/

Done.

http://codereview.chromium.org/1134002/diff/38001/39023#newcode494
chrome/browser/sync/resources/gaia_login.html:494:
style="width:150;visibility:hidden" disabled="true"/>
On 2010/03/30 20:21:53, arv wrote:
> <input ... >
> 
> no />

Done.

http://codereview.chromium.org/1134002/diff/38001/39025
File chrome/browser/sync/resources/setup_flow.html (right):

http://codereview.chromium.org/1134002/diff/38001/39025#newcode1
chrome/browser/sync/resources/setup_flow.html:1: <HTML id='t'>
On 2010/03/30 20:21:53, arv wrote:
> Use lowercase tag names

Done.

http://codereview.chromium.org/1134002/diff/38001/39025#newcode1
chrome/browser/sync/resources/setup_flow.html:1: <HTML id='t'>
On 2010/03/30 20:21:53, arv wrote:
> Missing doctype

What should it be?  
<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01//EN"
"http://www.w3.org/TR/html4/strict.dtd"> ?

http://codereview.chromium.org/1134002/diff/38001/39025#newcode6
chrome/browser/sync/resources/setup_flow.html:6:
document.getElementById("login").style.display = "none";
On 2010/03/30 20:21:53, arv wrote:
> use single quotes in js

Done.

[arv (Not doing code reviews), 2010-03-30 22:28:56]

On Tue, Mar 30, 2010 at 14:54,  <[email protected]> wrote:
> Thanks, Erik.  I've fixed most of the changes.  I'm not changing the doctype
> or
> the <!-- --> comments right now, because those might change the way things
> look;
> I'll do it in a later CL.
>
>
> http://codereview.chromium.org/1134002/diff/38001/39023
> File chrome/browser/sync/resources/gaia_login.html (right):
>
> http://codereview.chromium.org/1134002/diff/38001/39023#newcode4
> chrome/browser/sync/resources/gaia_login.html:4: body {
> bgcolor:"#ffffff" }
> On 2010/03/30 20:21:53, arv wrote:
>>
>> Invalid style property. Did you mean background-color>
>
> Done.
>
> http://codereview.chromium.org/1134002/diff/38001/39023#newcode4
> chrome/browser/sync/resources/gaia_login.html:4: body {
> bgcolor:"#ffffff" }
> On 2010/03/30 20:21:53, arv wrote:
>>
>> Invalid value. Did you mean #fff or #ffffff ?
>
> Done.
>
> http://codereview.chromium.org/1134002/diff/38001/39023#newcode493
> chrome/browser/sync/resources/gaia_login.html:493:
> i18n-values="value:customize" onClick="showCustomize();"
> On 2010/03/30 20:21:53, arv wrote:
>>
>> s/onClick/onclick/
>
> Done.
>
> http://codereview.chromium.org/1134002/diff/38001/39023#newcode494
> chrome/browser/sync/resources/gaia_login.html:494:
> style="width:150;visibility:hidden" disabled="true"/>
> On 2010/03/30 20:21:53, arv wrote:
>>
>> <input ... >
>
>> no />
>
> Done.
>
> http://codereview.chromium.org/1134002/diff/38001/39025
> File chrome/browser/sync/resources/setup_flow.html (right):
>
> http://codereview.chromium.org/1134002/diff/38001/39025#newcode1
> chrome/browser/sync/resources/setup_flow.html:1: <HTML id='t'>
> On 2010/03/30 20:21:53, arv wrote:
>>
>> Use lowercase tag names
>
> Done.
>
> http://codereview.chromium.org/1134002/diff/38001/39025#newcode1
> chrome/browser/sync/resources/setup_flow.html:1: <HTML id='t'>
> On 2010/03/30 20:21:53, arv wrote:
>>
>> Missing doctype
>
> What should it be?
> <!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01//EN"
> "http://www.w3.org/TR/html4/strict.dtd"> ?

<!DOCTYPE html>

> http://codereview.chromium.org/1134002/diff/38001/39025#newcode6
> chrome/browser/sync/resources/setup_flow.html:6:
> document.getElementById("login").style.display = "none";
> On 2010/03/30 20:21:53, arv wrote:
>>
>> use single quotes in js
>
> Done.
>
> http://codereview.chromium.org/1134002
>