Refactor LoginUIService to not rely on WebUI.

Refactor LoginUIService to not rely on WebUI.

Clean up LoginUIService::ShowLoginUI() API to not take a force_login flag.
Moved force_login code into the only place that needs it - AppNotifyChannelUI.

Fixed AppNotifyChannelUI code to gracefully handle having sync UI visible when the
user tries to re-auth.

BUG=118795
TEST=bring up sync UI via various methods (promo, app notifications infobar, wrench menu, error link in settings page, etc)


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

[Andrew T Wilson (Slow), 2012-04-27 00:44:14]

James, here's that refactoring you requested back during your review a few weeks
ago. PTAL.

Munjal, PTAL at app/extensions/app_notify_ui_channel changes.

[James Hawkins, 2012-04-27 17:19:03]

http://codereview.chromium.org/10192005/diff/1/chrome/browser/extensions/app_...
File chrome/browser/extensions/app_notify_channel_ui.cc (right):

http://codereview.chromium.org/10192005/diff/1/chrome/browser/extensions/app_...
chrome/browser/extensions/app_notify_channel_ui.cc:136: // Let's bring up the
login page.
nit: Don't use pronouns in comments, since pronouns are ambiguous.  The pronoun
here is 'us'.

Here and elsewhere.

http://codereview.chromium.org/10192005/diff/1/chrome/browser/ui/webui/sync_s...
File chrome/browser/ui/webui/sync_setup_handler.cc (right):

http://codereview.chromium.org/10192005/diff/1/chrome/browser/ui/webui/sync_s...
chrome/browser/ui/webui/sync_setup_handler.cc:760: if
(!FocusExistingWizardIfPresent()) {
Optional nit: I believe the precedence in this file is to not use braces for
single-line logic blocks.

[Andrew T Wilson (Slow), 2012-04-28 00:32:27]

PTAL

https://chromiumcodereview.appspot.com/10192005/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/app_notify_channel_ui.cc (right):

https://chromiumcodereview.appspot.com/10192005/diff/1/chrome/browser/extensi...
chrome/browser/extensions/app_notify_channel_ui.cc:136: // Let's bring up the
login page.
On 2012/04/27 17:19:03, James Hawkins wrote:
> nit: Don't use pronouns in comments, since pronouns are ambiguous.  The
pronoun
> here is 'us'.
> 
> Here and elsewhere.

Done.

https://chromiumcodereview.appspot.com/10192005/diff/1/chrome/browser/ui/webu...
File chrome/browser/ui/webui/sync_setup_handler.cc (right):

https://chromiumcodereview.appspot.com/10192005/diff/1/chrome/browser/ui/webu...
chrome/browser/ui/webui/sync_setup_handler.cc:760: if
(!FocusExistingWizardIfPresent()) {
On 2012/04/27 17:19:03, James Hawkins wrote:
> Optional nit: I believe the precedence in this file is to not use braces for
> single-line logic blocks.

Done.

[James Hawkins, 2012-04-28 01:00:41]

lgtm

[Munjal (Google), 2012-04-30 17:44:17]

LGTM.

Did the unit tests for AppNotifyChannelUI pass? I am a bit surprised that
despite changing the code the unit tests did not have to change.

[commit-bot: I haz the power, 2012-05-01 00:22:56]

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

[commit-bot: I haz the power, 2012-05-01 00:23:04]

Presubmit check for 10192005-10001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/ui

Presubmit checks took 1.4s to calculate.

[Andrew T Wilson (Slow), 2012-05-01 00:38:49]

On 2012/04/30 17:44:17, munjal wrote:
> LGTM.
> 
> Did the unit tests for AppNotifyChannelUI pass? I am a bit surprised that
> despite changing the code the unit tests did not have to change.

There are no AppNotifyChannelUI tests that I can find. This refactoring doesn't
change the behavior except in the case where we already have login UI open.

[Andrew T Wilson (Slow), 2012-05-01 00:39:47]

sky: Please take a look at change to ui/browser.cc

[sky, 2012-05-01 20:29:33]

LGTM

[commit-bot: I haz the power, 2012-05-01 20:50:18]

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

[commit-bot: I haz the power, 2012-05-01 21:19:08]

Failed to request the patch to try. Please note that binary filesare still
unsupported at the moment, this is being worked on.

Thanks for your patience.

HTTP Error 500: 
<html><head>
<meta http-equiv="content-type" content="text/html;charset=utf-8">
<title>500 Server Error</title>
</head>
<body text=#000000 bgcolor=#ffffff>
<h1>Error: Server Error</h1>
<h2>The server encountered an error and could not complete your request.<p>If
the problem persists, please <A
HREF="http://code.google.com/appengine/community.html">report</A> your problem
and mention this error message and the query that caused it.</h2>
<h2></h2>
</body></html>

[commit-bot: I haz the power, 2012-05-01 21:20:34]

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

[commit-bot: I haz the power, 2012-05-01 21:35:35]

Try job failure for 10192005-10001 on win_rel for step "update".
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-05-01 22:07:38]

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

[commit-bot: I haz the power, 2012-05-01 23:33:45]

Change committed as 134819