Remove PSS::ShowLoginDialog() and obsolete IDC_SYNC_BOOKMARKS.

Remove PSS::ShowLoginDialog() and obsolete IDC_SYNC_BOOKMARKS.

Changed calls to ShowLoginDialog() to go through LoginUIService() instead. Also noticed that
IDC_SYNC_BOOKMARKS has been superceded by IDC_SHOW_SYNC_SETUP so changed the remaining uses over so
we only have to maintain a single UI flow.

Modified MainMenu.xib to generate IDC_SHOW_SYNC_SETUP instead of IDC_SYNC_BOOKMARKS when the user clicks on the menu item.

TBR=jhawkins (for new_tab_page_sync_handler.cc which I believe is obsolete/unused)

BUG=111859
TEST=bring up login via wrench/app menu.


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

[Andrew T Wilson (Slow), 2012-02-24 00:56:30]

PTAL - touches a few different directories so I'm adding people for OWNERS
rubberstamps.

sky: chrome/browser/ui changes
sail: chrome/browser/ui/... changes + change to MainMenu.xib
dimich: can you rubberstamp changes to browser/ui/panels?
akalin: browser/sync + whatever else you want to look at (since I think you had
your fingers in some of this stuff previously)

[sky, 2012-02-24 01:13:36]

LGTM

http://codereview.chromium.org/9456031/diff/1/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/9456031/diff/1/chrome/browser/ui/browser.cc#ne...
chrome/browser/ui/browser.cc:936: void Browser::OpenSyncSetupWindow(Profile*
profile) {
Make position match header.

[sail, 2012-02-24 01:27:37]

xib changes and ui/* changes look good to me.
You'll want to describe the xib changes in your CL description.

http://codereview.chromium.org/9456031/diff/1/chrome/browser/ui/webui/ntp/new...
File chrome/browser/ui/webui/ntp/new_tab_page_sync_handler.cc (right):

http://codereview.chromium.org/9456031/diff/1/chrome/browser/ui/webui/ntp/new...
chrome/browser/ui/webui/ntp/new_tab_page_sync_handler.cc:128:
ProfileSyncService::SyncEvent(ProfileSyncService::START_FROM_NTP);
Do you think we could make this match Browser::ShowSyncSetup() instead?

[sail, 2012-02-24 01:27:45]

lgtm

[Andrew T Wilson (Slow), 2012-02-24 01:49:37]

http://codereview.chromium.org/9456031/diff/1/chrome/browser/ui/webui/ntp/new...
File chrome/browser/ui/webui/ntp/new_tab_page_sync_handler.cc (right):

http://codereview.chromium.org/9456031/diff/1/chrome/browser/ui/webui/ntp/new...
chrome/browser/ui/webui/ntp/new_tab_page_sync_handler.cc:128:
ProfileSyncService::SyncEvent(ProfileSyncService::START_FROM_NTP);
Can you clarify what you mean? Are you saying that we should *call*
Browser::ShowSyncSetup() and get rid of all this code? Or something else?

[akalin, 2012-02-24 01:56:12]

LGTM, although I think everyone else is more qualified to review

http://codereview.chromium.org/9456031/diff/1/chrome/browser/app_controller_m...
File chrome/browser/app_controller_mac.mm (right):

http://codereview.chromium.org/9456031/diff/1/chrome/browser/app_controller_m...
chrome/browser/app_controller_mac.mm:910: if (Browser* browser =
ActivateBrowser(lastProfile))
almost certain this is against style. what about:

case IDC...: {
  Browser* browser = ...;
  if (browser) {
     ..
  } else {
     ..
  }
}

[akalin, 2012-02-24 01:58:09]

http://codereview.chromium.org/9456031/diff/1/chrome/browser/app_controller_m...
> File chrome/browser/app_controller_mac.mm (right):
> 
>
http://codereview.chromium.org/9456031/diff/1/chrome/browser/app_controller_m...
> chrome/browser/app_controller_mac.mm:910: if (Browser* browser =
> ActivateBrowser(lastProfile))
> almost certain this is against style. what about:
> 
> case IDC...: {
>   Browser* browser = ...;
>   if (browser) {
>      ..
>   } else {
>      ..
>   }
> }


Didn't see that this matches the surrounding style.  Nevermind this comment.

[Andrew T Wilson (Slow), 2012-02-24 22:12:11]

https://chromiumcodereview.appspot.com/9456031/diff/1/chrome/browser/app_cont...
File chrome/browser/app_controller_mac.mm (right):

https://chromiumcodereview.appspot.com/9456031/diff/1/chrome/browser/app_cont...
chrome/browser/app_controller_mac.mm:910: if (Browser* browser =
ActivateBrowser(lastProfile))
not changing per our discussion.

https://chromiumcodereview.appspot.com/9456031/diff/1/chrome/browser/ui/brows...
File chrome/browser/ui/browser.cc (right):

https://chromiumcodereview.appspot.com/9456031/diff/1/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:936: void Browser::OpenSyncSetupWindow(Profile*
profile) {
On 2012/02/24 01:13:36, sky wrote:
> Make position match header.

Done.

https://chromiumcodereview.appspot.com/9456031/diff/1/chrome/browser/ui/webui...
File chrome/browser/ui/webui/ntp/new_tab_page_sync_handler.cc (right):

https://chromiumcodereview.appspot.com/9456031/diff/1/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/new_tab_page_sync_handler.cc:128:
ProfileSyncService::SyncEvent(ProfileSyncService::START_FROM_NTP);
Changed this to call ShowSyncSetup() as requested.

Also, I think this file may be obsolete now that we're using ntp4 (nobody seems
to generate the SyncLinkClicked event that calls this code) anymore. Sailesh,
can you confirm and either delete this file or test to make sure the previous
behavior continues to work?

[commit-bot: I haz the power, 2012-02-24 22:12:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/atwilson@chromium.org/9456031/7001

[commit-bot: I haz the power, 2012-02-24 22:12:46]

Presubmit check for 9456031-7001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
chrome/browser/ui/webui/ntp/new_tab_page_sync_handler.cc,chrome/browser/ui/panels/panel_window_controller_cocoa.mm,chrome/browser/ui/panels/panel_browser_window_cocoa_unittest.mm

Presubmit checks took 1.9s to calculate.

[jianli, 2012-02-24 22:53:07]

LGTM on panel changes.

[commit-bot: I haz the power, 2012-02-24 22:59:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/atwilson@chromium.org/9456031/7001

[commit-bot: I haz the power, 2012-02-25 00:45:43]

Change committed as 123610