Add a method to LoginUIService to open the sign in UI in a popup.

chrome/common/chrome_notification_types.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CHROME_COMMON_CHROME_NOTIFICATION_TYPES_H_
 #define CHROME_COMMON_CHROME_NOTIFICATION_TYPES_H_
 #pragma once
 
 #include "content/public/browser/notification_types.h"
 
@@ skipping 852 matching lines @@
 
   // Sent when a user fails to sign into Google services such as sync.
   // The source is the Profile. The details are a GoogleServiceAuthError
   // object.
   NOTIFICATION_GOOGLE_SIGNIN_FAILED,
 
   // Sent when the currently signed-in user for a user has been signed out.
   // The source is the Profile. There are no details.
   NOTIFICATION_GOOGLE_SIGNED_OUT,
 
+  // Sent when a login UI state changes: when new login UI is shown or when
+  // login UI is closed.
+  NOTIFICATION_LOGIN_UI_CHANGED,

[jam, 2012/07/03 05:52:27]

who's listening to this notification?

[Munjal (Google), 2012/07/03 17:52:17]

The listening code is coming up in the next patch.

[jam, 2012/07/04 04:25:21]

can we keep the notification until that patch then? I would like to see what the
code looks like. I generally dislike notifications when used to get from one
place to another, because often that can be accomplished with delegate/listener
interfaces or callbacks instead. 

[Munjal (Google), 2012/07/09 17:30:06]

I uploaded the patch that uses the notification. See
https://chromiumcodereview.appspot.com/10701041

Note that it is not ready for review yet.

I considered delegate/listener pattern here since even I prefer that over
notifications in a lot of cases. In this case though, notification seemed more
appropriate. But take a look at that patch and let me know what you think.

[jam, 2012/07/09 17:35:53]

given that there's only one place that listens to this notification, and that
code can easily get to the LoginUIService which fires this notification, it
would be preferable to avoid adding yet another notification imo.

i'm curious why you say a notification would be more appropriate in this case?

[Munjal (Google), 2012/07/09 19:24:59]

The main reason is to simplify lifetime management of SigninDialog instance. If
it is a notification, then we don't have to worry about one object out-living
another. If not, then I have ot make sure SigninDialog lifetime is managed
appropriately.

That patch does not yet contain the code that uses SigninDialog. It will be used
in an extension functino implementation. Once that is hooked up, if we use a
delegate pattern the call order would be: LoginUIService ->
SigninDialog::OnDialogClosed -> Extension Function's OnSigninDone -> ~Extension
Function -> ~SigninDialog -> unsubscribe from LoginUIService.

So the SigninDialog will be deleeted in response to the delegate callback which
in turn unsubscribe (again within the delegate callback context).

All of that can be made to work, but seemed very fragile.

[jam, 2012/07/09 19:38:13]

Given that LoginUIService is a profile keyed service, and profiles live until
browser shutdown, I don't see the lifetime issue?

[Munjal (Google), 2012/07/09 19:48:54]

Yeah, LoginUIService is okay. You are right.

But the other way around seemed a bit fragile - the fact that delegate instance
will be deleted inside the call to delegate which will in turn unsubscribe from
LoginUIService. it felt a bit awkward.

Note that SigninDialog (in the other patch) exposes a delegate pattern and
listens to the notification. So it is sort of an adapter from notification to
delegate. But because it takes delegate in the constructor and it lives in
extension code base, it felt like a more appropriate design.

Having said all of that, , if you prefer delegate style directly with
LoginUIService, I can do that. One more note about using delegate with
LoginUIService - it will need to support multiple delegates - AddObserver and
RemoveObserver like methods.

[jam, 2012/07/09 21:02:50]

This is a common pattern, and we have ObserverList precisely to solve this case.

+
   // Autofill Notifications --------------------------------------------------
 
   // Sent when a popup with Autofill suggestions is shown in the renderer.
   // The source is the corresponding RenderViewHost. There are not details.
   NOTIFICATION_AUTOFILL_DID_SHOW_SUGGESTIONS,
 
   // Sent when a form is previewed or filled with Autofill suggestions.
   // The source is the corresponding RenderViewHost. There are not details.
   NOTIFICATION_AUTOFILL_DID_FILL_FORM_DATA,
 
@@ skipping 300 matching lines @@
   // Currently only Content and Chrome define and use notifications.
   // Custom notifications not belonging to Content and Chrome should start
   // from here.
   NOTIFICATION_CHROME_END,
 };
 
 }  // namespace chrome
 
 
 #endif  // CHROME_COMMON_CHROME_NOTIFICATION_TYPES_H_