Sync Promo: Add a way to collapse the sync promo

Sync Promo: Add a way to collapse the sync promo

With this change we can now collapse and expand the sync promo.

BUG=89276
TEST=Ran with --sync-show-promo and verified that collapsing and un-collapsing the sync promo worked.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=93585

[James Hawkins, 2011-07-18 02:37:36]

http://codereview.chromium.org/7399015/diff/14/chrome/browser/resources/new_t...
File chrome/browser/resources/new_tab.html (right):

http://codereview.chromium.org/7399015/diff/14/chrome/browser/resources/new_t...
chrome/browser/resources/new_tab.html:144: src="sync_promo_minimize.png"/>
s/\///

http://codereview.chromium.org/7399015/diff/14/chrome/browser/resources/new_t...
File chrome/browser/resources/new_tab_sync_promo.css (right):

http://codereview.chromium.org/7399015/diff/14/chrome/browser/resources/new_t...
chrome/browser/resources/new_tab_sync_promo.css:8: width:100%;
Space after colon, here and elsewhere.

http://codereview.chromium.org/7399015/diff/14/chrome/browser/resources/new_t...
chrome/browser/resources/new_tab_sync_promo.css:9: background-color: #F6F6F6;
Lower-case letters for hex color.

http://codereview.chromium.org/7399015/diff/14/chrome/browser/resources/new_t...
chrome/browser/resources/new_tab_sync_promo.css:9: background-color: #F6F6F6;
Alphabetize properties, here and elsewhere.

http://codereview.chromium.org/7399015/diff/14/chrome/browser/resources/new_t...
File chrome/browser/resources/new_tab_sync_promo.js (right):

http://codereview.chromium.org/7399015/diff/14/chrome/browser/resources/new_t...
chrome/browser/resources/new_tab_sync_promo.js:7: var isExpanded_ = false;
Var name not descriptive enough for the context. isSyncPromoExpanded_ is
probably better.

http://codereview.chromium.org/7399015/diff/14/chrome/browser/resources/new_t...
chrome/browser/resources/new_tab_sync_promo.js:34:
$('sync-promo-toggle-button').onclick = function() {
onclick = this.onTogglePromo;

http://codereview.chromium.org/7399015/diff/14/chrome/browser/resources/new_t...
chrome/browser/resources/new_tab_sync_promo.js:41: onTogglePromo: function() {
Document all functions.

http://codereview.chromium.org/7399015/diff/14/chrome/browser/resources/new_t...
chrome/browser/resources/new_tab_sync_promo.js:53:
$('sync-promo-login-status').hidden = true;
These hide/shows should be animated (specified in the CSS).

http://codereview.chromium.org/7399015/diff/14/chrome/browser/resources/new_t...
chrome/browser/resources/new_tab_sync_promo.js:107: $('sync-promo').hidden =
false;
Delegate to a private method in NewTabSyncPromo, as above.

http://codereview.chromium.org/7399015/diff/14/chrome/browser/ui/webui/ntp/ne...
File chrome/browser/ui/webui/ntp/new_tab_sync_setup_handler.cc (right):

http://codereview.chromium.org/7399015/diff/14/chrome/browser/ui/webui/ntp/ne...
chrome/browser/ui/webui/ntp/new_tab_sync_setup_handler.cc:17:
WebUIMessageHandler* NewTabSyncSetupHandler::Attach(WebUI* web_ui) {
Ordering in implementation should match the ordering in the class declaration.

http://codereview.chromium.org/7399015/diff/14/chrome/browser/ui/webui/ntp/ne...
chrome/browser/ui/webui/ntp/new_tab_sync_setup_handler.cc:54: #if
defined(OS_CHROMEOS)
Document why this is ifdef'ed out on cros.

http://codereview.chromium.org/7399015/diff/14/chrome/browser/ui/webui/ntp/ne...
File chrome/browser/ui/webui/ntp/new_tab_sync_setup_handler.h (right):

http://codereview.chromium.org/7399015/diff/14/chrome/browser/ui/webui/ntp/ne...
chrome/browser/ui/webui/ntp/new_tab_sync_setup_handler.h:11: class
NewTabSyncSetupHandler : public SyncSetupHandler {
Document the class.

http://codereview.chromium.org/7399015/diff/14/chrome/browser/ui/webui/ntp/ne...
chrome/browser/ui/webui/ntp/new_tab_sync_setup_handler.h:13: static bool
ShouldShowSyncPromo();
Document this method.

http://codereview.chromium.org/7399015/diff/14/chrome/browser/ui/webui/ntp/ne...
chrome/browser/ui/webui/ntp/new_tab_sync_setup_handler.h:15: virtual
WebUIMessageHandler* Attach(WebUI* web_ui);
// XYZ implementation.

For whatever the correct XYZ is (I don't know off the top of my head.

http://codereview.chromium.org/7399015/diff/14/chrome/browser/ui/webui/ntp/ne...
chrome/browser/ui/webui/ntp/new_tab_sync_setup_handler.h:18: //
NotificationObserver interface
// NotificationObserver implementation.

http://codereview.chromium.org/7399015/diff/14/chrome/browser/ui/webui/ntp/ne...
chrome/browser/ui/webui/ntp/new_tab_sync_setup_handler.h:26: void
HandleInitializeSyncPromo(const ListValue* args);
Document all of these methods.

http://codereview.chromium.org/7399015/diff/14/chrome/browser/ui/webui/ntp/ne...
chrome/browser/ui/webui/ntp/new_tab_sync_setup_handler.h:33: StringPrefMember
username_pref_;
Document member var.

http://codereview.chromium.org/7399015/diff/14/chrome/browser/ui/webui/sync_s...
File chrome/browser/ui/webui/sync_setup_handler.h (right):

http://codereview.chromium.org/7399015/diff/14/chrome/browser/ui/webui/sync_s...
chrome/browser/ui/webui/sync_setup_handler.h:37: void OpenSyncSetup();
Documentation.

[sail, 2011-07-20 01:30:46]

http://codereview.chromium.org/7399015/diff/14/chrome/browser/resources/new_t...
File chrome/browser/resources/new_tab.html (right):

http://codereview.chromium.org/7399015/diff/14/chrome/browser/resources/new_t...
chrome/browser/resources/new_tab.html:144: src="sync_promo_minimize.png"/>
On 2011/07/18 02:37:36, James Hawkins wrote:
> s/\///

Done.

http://codereview.chromium.org/7399015/diff/14/chrome/browser/resources/new_t...
File chrome/browser/resources/new_tab_sync_promo.css (right):

http://codereview.chromium.org/7399015/diff/14/chrome/browser/resources/new_t...
chrome/browser/resources/new_tab_sync_promo.css:8: width:100%;
On 2011/07/18 02:37:36, James Hawkins wrote:
> Space after colon, here and elsewhere.

Done.

http://codereview.chromium.org/7399015/diff/14/chrome/browser/resources/new_t...
chrome/browser/resources/new_tab_sync_promo.css:9: background-color: #F6F6F6;
On 2011/07/18 02:37:36, James Hawkins wrote:
> Lower-case letters for hex color.

Done.

http://codereview.chromium.org/7399015/diff/14/chrome/browser/resources/new_t...
chrome/browser/resources/new_tab_sync_promo.css:9: background-color: #F6F6F6;
On 2011/07/18 02:37:36, James Hawkins wrote:
> Lower-case letters for hex color.

Done.

http://codereview.chromium.org/7399015/diff/14/chrome/browser/resources/new_t...
File chrome/browser/resources/new_tab_sync_promo.js (right):

http://codereview.chromium.org/7399015/diff/14/chrome/browser/resources/new_t...
chrome/browser/resources/new_tab_sync_promo.js:7: var isExpanded_ = false;
On 2011/07/18 02:37:36, James Hawkins wrote:
> Var name not descriptive enough for the context. isSyncPromoExpanded_ is
> probably better.

Done.

http://codereview.chromium.org/7399015/diff/14/chrome/browser/resources/new_t...
chrome/browser/resources/new_tab_sync_promo.js:34:
$('sync-promo-toggle-button').onclick = function() {
On 2011/07/18 02:37:36, James Hawkins wrote:
> onclick = this.onTogglePromo;

Done.

http://codereview.chromium.org/7399015/diff/14/chrome/browser/resources/new_t...
chrome/browser/resources/new_tab_sync_promo.js:41: onTogglePromo: function() {
On 2011/07/18 02:37:36, James Hawkins wrote:
> Document all functions.

Done.

http://codereview.chromium.org/7399015/diff/14/chrome/browser/resources/new_t...
chrome/browser/resources/new_tab_sync_promo.js:53:
$('sync-promo-login-status').hidden = true;
On 2011/07/18 02:37:36, James Hawkins wrote:
> These hide/shows should be animated (specified in the CSS).

Mind if I do this in a separate change? This UI is going to change slightly in
the next CL and it'll be easier to add animations after that. Thanks

http://codereview.chromium.org/7399015/diff/14/chrome/browser/resources/new_t...
chrome/browser/resources/new_tab_sync_promo.js:107: $('sync-promo').hidden =
false;
On 2011/07/18 02:37:36, James Hawkins wrote:
> Delegate to a private method in NewTabSyncPromo, as above.

Done.

http://codereview.chromium.org/7399015/diff/14/chrome/browser/ui/webui/ntp/ne...
File chrome/browser/ui/webui/ntp/new_tab_sync_setup_handler.cc (right):

http://codereview.chromium.org/7399015/diff/14/chrome/browser/ui/webui/ntp/ne...
chrome/browser/ui/webui/ntp/new_tab_sync_setup_handler.cc:17:
WebUIMessageHandler* NewTabSyncSetupHandler::Attach(WebUI* web_ui) {
On 2011/07/18 02:37:36, James Hawkins wrote:
> Ordering in implementation should match the ordering in the class declaration.

Done.

http://codereview.chromium.org/7399015/diff/14/chrome/browser/ui/webui/ntp/ne...
chrome/browser/ui/webui/ntp/new_tab_sync_setup_handler.cc:54: #if
defined(OS_CHROMEOS)
On 2011/07/18 02:37:36, James Hawkins wrote:
> Document why this is ifdef'ed out on cros.

Done.

http://codereview.chromium.org/7399015/diff/14/chrome/browser/ui/webui/ntp/ne...
File chrome/browser/ui/webui/ntp/new_tab_sync_setup_handler.h (right):

http://codereview.chromium.org/7399015/diff/14/chrome/browser/ui/webui/ntp/ne...
chrome/browser/ui/webui/ntp/new_tab_sync_setup_handler.h:11: class
NewTabSyncSetupHandler : public SyncSetupHandler {
On 2011/07/18 02:37:36, James Hawkins wrote:
> Document the class.

Done.

http://codereview.chromium.org/7399015/diff/14/chrome/browser/ui/webui/ntp/ne...
chrome/browser/ui/webui/ntp/new_tab_sync_setup_handler.h:13: static bool
ShouldShowSyncPromo();
On 2011/07/18 02:37:36, James Hawkins wrote:
> Document this method.

Done.

http://codereview.chromium.org/7399015/diff/14/chrome/browser/ui/webui/ntp/ne...
chrome/browser/ui/webui/ntp/new_tab_sync_setup_handler.h:15: virtual
WebUIMessageHandler* Attach(WebUI* web_ui);
On 2011/07/18 02:37:36, James Hawkins wrote:
> // XYZ implementation.
> 
> For whatever the correct XYZ is (I don't know off the top of my head.

Done.

http://codereview.chromium.org/7399015/diff/14/chrome/browser/ui/webui/ntp/ne...
chrome/browser/ui/webui/ntp/new_tab_sync_setup_handler.h:18: //
NotificationObserver interface
On 2011/07/18 02:37:36, James Hawkins wrote:
> // NotificationObserver implementation.

Done.

http://codereview.chromium.org/7399015/diff/14/chrome/browser/ui/webui/ntp/ne...
chrome/browser/ui/webui/ntp/new_tab_sync_setup_handler.h:26: void
HandleInitializeSyncPromo(const ListValue* args);
On 2011/07/18 02:37:36, James Hawkins wrote:
> Document all of these methods.

Done.

http://codereview.chromium.org/7399015/diff/14/chrome/browser/ui/webui/ntp/ne...
chrome/browser/ui/webui/ntp/new_tab_sync_setup_handler.h:33: StringPrefMember
username_pref_;
On 2011/07/18 02:37:36, James Hawkins wrote:
> Document member var.

Done.

http://codereview.chromium.org/7399015/diff/14/chrome/browser/ui/webui/sync_s...
File chrome/browser/ui/webui/sync_setup_handler.h (right):

http://codereview.chromium.org/7399015/diff/14/chrome/browser/ui/webui/sync_s...
chrome/browser/ui/webui/sync_setup_handler.h:37: void OpenSyncSetup();
On 2011/07/18 02:37:36, James Hawkins wrote:
> Documentation.

Done.

[James Hawkins, 2011-07-20 01:55:25]

LGTM with nits.

http://codereview.chromium.org/7399015/diff/8001/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_pref_provider.cc (right):

http://codereview.chromium.org/7399015/diff/8001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider.cc:1034: if
(key.empty())
When does this happen?

http://codereview.chromium.org/7399015/diff/8001/chrome/browser/resources/new...
File chrome/browser/resources/new_tab.html (right):

http://codereview.chromium.org/7399015/diff/8001/chrome/browser/resources/new...
chrome/browser/resources/new_tab.html:329: <script
src="options/options_page.js"></script>
I don't know how we can work around this, but including options_page.js is no
bueno. We'll likely have to just add a TODO.

http://codereview.chromium.org/7399015/diff/8001/chrome/browser/resources/new...
File chrome/browser/resources/new_tab_sync_promo.js (right):

http://codereview.chromium.org/7399015/diff/8001/chrome/browser/resources/new...
chrome/browser/resources/new_tab_sync_promo.js:7: var isSyncPromoExpanded_ =
false;
Move this into NewTabSyncPromo.prototype.

http://codereview.chromium.org/7399015/diff/8001/chrome/browser/ui/webui/ntp/...
File chrome/browser/ui/webui/ntp/new_tab_sync_setup_handler.h (right):

http://codereview.chromium.org/7399015/diff/8001/chrome/browser/ui/webui/ntp/...
chrome/browser/ui/webui/ntp/new_tab_sync_setup_handler.h:40: void UpdateLogin();
Document method.

[sail, 2011-07-20 19:40:24]

http://codereview.chromium.org/7399015/diff/8001/chrome/browser/content_setti...
File chrome/browser/content_settings/content_settings_pref_provider.cc (right):

http://codereview.chromium.org/7399015/diff/8001/chrome/browser/content_setti...
chrome/browser/content_settings/content_settings_pref_provider.cc:1034: if
(key.empty())
On 2011/07/20 01:55:26, James Hawkins wrote:
> When does this happen?

Oops, this is a separate change. Removed.

http://codereview.chromium.org/7399015/diff/8001/chrome/browser/resources/new...
File chrome/browser/resources/new_tab.html (right):

http://codereview.chromium.org/7399015/diff/8001/chrome/browser/resources/new...
chrome/browser/resources/new_tab.html:329: <script
src="options/options_page.js"></script>
On 2011/07/20 01:55:26, James Hawkins wrote:
> I don't know how we can work around this, but including options_page.js is no
> bueno. We'll likely have to just add a TODO.

Done. Added TODO

http://codereview.chromium.org/7399015/diff/8001/chrome/browser/resources/new...
File chrome/browser/resources/new_tab_sync_promo.js (right):

http://codereview.chromium.org/7399015/diff/8001/chrome/browser/resources/new...
chrome/browser/resources/new_tab_sync_promo.js:7: var isSyncPromoExpanded_ =
false;
On 2011/07/20 01:55:26, James Hawkins wrote:
> Move this into NewTabSyncPromo.prototype.

Done.

http://codereview.chromium.org/7399015/diff/8001/chrome/browser/ui/webui/ntp/...
File chrome/browser/ui/webui/ntp/new_tab_sync_setup_handler.h (right):

http://codereview.chromium.org/7399015/diff/8001/chrome/browser/ui/webui/ntp/...
chrome/browser/ui/webui/ntp/new_tab_sync_setup_handler.h:40: void UpdateLogin();
On 2011/07/20 01:55:26, James Hawkins wrote:
> Document method.

Done.

http://codereview.chromium.org/7399015/diff/13001/chrome/browser/resources/ne...
File chrome/browser/resources/new_tab_sync_promo.js (right):

http://codereview.chromium.org/7399015/diff/13001/chrome/browser/resources/ne...
chrome/browser/resources/new_tab_sync_promo.js:34:
$('sync-promo-toggle-button').onclick = function() {
I needed to change the handler back to an anonymous function otherwise I the
value of this in onTogglePromo was wrong.

[sail, 2011-07-20 19:41:06]

fixed parent branch

[James Hawkins, 2011-07-20 19:59:46]

SLGTM