Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10699013:
Add a method to LoginUIService to open the sign in UI in a popup. (Closed)
|
Created:
8 years, 5 months ago by Munjal (Google) Modified:
8 years, 5 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionAdd ability to add/remove observers for login ui shown/closed
to LoginUIService. This will be used by the extension API to show
login UI in a popup and then obseve when it is closed and respond back
to the extension accordingly.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=146143
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #
Total comments: 16
Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #
Total comments: 6
Patch Set 7 : #
Total comments: 6
Patch Set 8 : #
Total comments: 12
Patch Set 9 : #
Total comments: 8
Patch Set 10 : #Patch Set 11 : #
Messages
Total messages: 36 (0 generated)
John has a conflicting patch on the way, so I think your approach will need to change: http://codereview.chromium.org/10692021/
Can you take another look? I merged in John's changes and I also changed the description of the patch to match better what I am doing in this patch.
http://codereview.chromium.org/10699013/diff/9001/chrome/common/chrome_notifi... File chrome/common/chrome_notification_types.h (right): http://codereview.chromium.org/10699013/diff/9001/chrome/common/chrome_notifi... chrome/common/chrome_notification_types.h:875: NOTIFICATION_LOGIN_UI_CHANGED, who's listening to this notification?
http://codereview.chromium.org/10699013/diff/9001/chrome/browser/ui/webui/sig... File chrome/browser/ui/webui/signin/login_ui_service.cc (right): http://codereview.chromium.org/10699013/diff/9001/chrome/browser/ui/webui/sig... chrome/browser/ui/webui/signin/login_ui_service.cc:52: void LoginUIService::ShowLoginDialogUI(Browser* browser) { This isn't displaying a dialog - it's displaying a popup. Does it make sense to just allow the caller to pass in the type of WindowOpenDisposition, for added flexibility? Also, I'm not at all sure that this is so generally useful that we need to make a common API for it. If it's only going to get called from one place, can we move this functionality into the caller instead? http://codereview.chromium.org/10699013/diff/9001/chrome/browser/ui/webui/sig... chrome/browser/ui/webui/signin/login_ui_service.cc:69: content::NotificationService::NoDetails()); Does it make sense to send out the current login UI with this notification?
Also, needs a test to show that the notification is actually being sent.
Working on adding a test. Will publish again when done. http://codereview.chromium.org/10699013/diff/9001/chrome/browser/ui/webui/sig... File chrome/browser/ui/webui/signin/login_ui_service.cc (right): http://codereview.chromium.org/10699013/diff/9001/chrome/browser/ui/webui/sig... chrome/browser/ui/webui/signin/login_ui_service.cc:52: void LoginUIService::ShowLoginDialogUI(Browser* browser) { On 2012/07/03 17:34:09, Andrew T Wilson wrote: > This isn't displaying a dialog - it's displaying a popup. > > Does it make sense to just allow the caller to pass in the type of > WindowOpenDisposition, for added flexibility? > > Also, I'm not at all sure that this is so generally useful that we need to make > a common API for it. If it's only going to get called from one place, can we > move this functionality into the caller instead? I changed the name to ShowLoginUIPopup. I think it is better to not let the calelrs pass the windowOpenDisposition yet. I don't mind moving it to the caller. But I think this is a better place since the caller has to do the same stuff about focusing existing UI if present etc. and for that it integrates with LoginUIService anyway. http://codereview.chromium.org/10699013/diff/9001/chrome/browser/ui/webui/sig... chrome/browser/ui/webui/signin/login_ui_service.cc:69: content::NotificationService::NoDetails()); On 2012/07/03 17:34:09, Andrew T Wilson wrote: > Does it make sense to send out the current login UI with this notification? We discussed that remember? In fact, your suggestion was to let hte callers query LoginUIService for current login UI. I liked it :). http://codereview.chromium.org/10699013/diff/9001/chrome/common/chrome_notifi... File chrome/common/chrome_notification_types.h (right): http://codereview.chromium.org/10699013/diff/9001/chrome/common/chrome_notifi... chrome/common/chrome_notification_types.h:875: NOTIFICATION_LOGIN_UI_CHANGED, On 2012/07/03 05:52:27, John Abd-El-Malek wrote: > who's listening to this notification? The listening code is coming up in the next patch.
http://codereview.chromium.org/10699013/diff/9001/chrome/common/chrome_notifi... File chrome/common/chrome_notification_types.h (right): http://codereview.chromium.org/10699013/diff/9001/chrome/common/chrome_notifi... chrome/common/chrome_notification_types.h:875: NOTIFICATION_LOGIN_UI_CHANGED, On 2012/07/03 17:52:17, munjal wrote: > On 2012/07/03 05:52:27, John Abd-El-Malek wrote: > > who's listening to this notification? > > The listening code is coming up in the next patch. 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.
I added tests now. Please take another look. http://codereview.chromium.org/10699013/diff/9001/chrome/common/chrome_notifi... File chrome/common/chrome_notification_types.h (right): http://codereview.chromium.org/10699013/diff/9001/chrome/common/chrome_notifi... chrome/common/chrome_notification_types.h:875: NOTIFICATION_LOGIN_UI_CHANGED, On 2012/07/04 04:25:21, John Abd-El-Malek wrote: > On 2012/07/03 17:52:17, munjal wrote: > > On 2012/07/03 05:52:27, John Abd-El-Malek wrote: > > > who's listening to this notification? > > > > The listening code is coming up in the next patch. > > 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. 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.
http://codereview.chromium.org/10699013/diff/9001/chrome/common/chrome_notifi... File chrome/common/chrome_notification_types.h (right): http://codereview.chromium.org/10699013/diff/9001/chrome/common/chrome_notifi... chrome/common/chrome_notification_types.h:875: NOTIFICATION_LOGIN_UI_CHANGED, On 2012/07/09 17:30:06, munjal wrote: > On 2012/07/04 04:25:21, John Abd-El-Malek wrote: > > On 2012/07/03 17:52:17, munjal wrote: > > > On 2012/07/03 05:52:27, John Abd-El-Malek wrote: > > > > who's listening to this notification? > > > > > > The listening code is coming up in the next patch. > > > > 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. > > 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. 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?
http://codereview.chromium.org/10699013/diff/9001/chrome/common/chrome_notifi... File chrome/common/chrome_notification_types.h (right): http://codereview.chromium.org/10699013/diff/9001/chrome/common/chrome_notifi... chrome/common/chrome_notification_types.h:875: NOTIFICATION_LOGIN_UI_CHANGED, On 2012/07/09 17:35:53, John Abd-El-Malek wrote: > On 2012/07/09 17:30:06, munjal wrote: > > On 2012/07/04 04:25:21, John Abd-El-Malek wrote: > > > On 2012/07/03 17:52:17, munjal wrote: > > > > On 2012/07/03 05:52:27, John Abd-El-Malek wrote: > > > > > who's listening to this notification? > > > > > > > > The listening code is coming up in the next patch. > > > > > > 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. > > > > 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. > > 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? 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.
http://codereview.chromium.org/10699013/diff/9001/chrome/common/chrome_notifi... File chrome/common/chrome_notification_types.h (right): http://codereview.chromium.org/10699013/diff/9001/chrome/common/chrome_notifi... chrome/common/chrome_notification_types.h:875: NOTIFICATION_LOGIN_UI_CHANGED, On 2012/07/09 19:24:59, munjal wrote: > On 2012/07/09 17:35:53, John Abd-El-Malek wrote: > > On 2012/07/09 17:30:06, munjal wrote: > > > On 2012/07/04 04:25:21, John Abd-El-Malek wrote: > > > > On 2012/07/03 17:52:17, munjal wrote: > > > > > On 2012/07/03 05:52:27, John Abd-El-Malek wrote: > > > > > > who's listening to this notification? > > > > > > > > > > The listening code is coming up in the next patch. > > > > > > > > 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. > > > > > > 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. > > > > 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? > > 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. Given that LoginUIService is a profile keyed service, and profiles live until browser shutdown, I don't see the lifetime issue? > > 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.
http://codereview.chromium.org/10699013/diff/9001/chrome/common/chrome_notifi... File chrome/common/chrome_notification_types.h (right): http://codereview.chromium.org/10699013/diff/9001/chrome/common/chrome_notifi... chrome/common/chrome_notification_types.h:875: NOTIFICATION_LOGIN_UI_CHANGED, On 2012/07/09 19:38:13, John Abd-El-Malek wrote: > On 2012/07/09 19:24:59, munjal wrote: > > On 2012/07/09 17:35:53, John Abd-El-Malek wrote: > > > On 2012/07/09 17:30:06, munjal wrote: > > > > On 2012/07/04 04:25:21, John Abd-El-Malek wrote: > > > > > On 2012/07/03 17:52:17, munjal wrote: > > > > > > On 2012/07/03 05:52:27, John Abd-El-Malek wrote: > > > > > > > who's listening to this notification? > > > > > > > > > > > > The listening code is coming up in the next patch. > > > > > > > > > > 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. > > > > > > > > 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. > > > > > > 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? > > > > 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. > > Given that LoginUIService is a profile keyed service, and profiles live until > browser shutdown, I don't see the lifetime issue? > > > > > 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. > 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.
Sorry, Forgot to send this last week. http://codereview.chromium.org/10699013/diff/9001/chrome/browser/ui/webui/sig... File chrome/browser/ui/webui/signin/login_ui_service.cc (right): http://codereview.chromium.org/10699013/diff/9001/chrome/browser/ui/webui/sig... chrome/browser/ui/webui/signin/login_ui_service.cc:52: void LoginUIService::ShowLoginDialogUI(Browser* browser) { On 2012/07/03 17:52:17, munjal wrote: > On 2012/07/03 17:34:09, Andrew T Wilson wrote: > > This isn't displaying a dialog - it's displaying a popup. > > > > Does it make sense to just allow the caller to pass in the type of > > WindowOpenDisposition, for added flexibility? > > > > Also, I'm not at all sure that this is so generally useful that we need to > make > > a common API for it. If it's only going to get called from one place, can we > > move this functionality into the caller instead? > > I changed the name to ShowLoginUIPopup. I think it is better to not let the > calelrs pass the windowOpenDisposition yet. > > I don't mind moving it to the caller. But I think this is a better place since > the caller has to do the same stuff about focusing existing UI if present etc. > and for that it integrates with LoginUIService anyway. Let's put this in the caller (take it out of this patch). In fact, given that ShowLoginUI() is so simple now (all the logic has been moved out to chrome::ShowSettingsSubPage()) I'm planning to remove that UI too. http://codereview.chromium.org/10699013/diff/9001/chrome/browser/ui/webui/sig... chrome/browser/ui/webui/signin/login_ui_service.cc:69: content::NotificationService::NoDetails()); Yeah, I remember, and I was just reconsidering the question. I don't have a strong opinion so I'm OK with whatever you decide.
BTW, John - why do you prefer notifications over listeners/delegates? It just seems like it's much more wiring required for listeners/delegates, especially when there's just a single call being generated (not a set of notifications). So, for something like "LoginUIService changed", I prefer a notification, whereas for something like SigninTracker which has multiple possible states (SigninSuccessful, SigninFailed, GaiaAuthenticationComplete) I prefer listeners/delegates. The only two advantages of listeners/delegates that I know of are that listeners/delegates let you encapsulate multiple API calls, and you get more type safety (not really an issue for a simple boolean/typeless notification). Are there other issues that make listeners preferable?
On 2012/07/09 19:55:56, Andrew T Wilson wrote: > BTW, John - why do you prefer notifications over listeners/delegates? It just > seems like it's much more wiring required for listeners/delegates, especially > when there's just a single call being generated (not a set of notifications). > > So, for something like "LoginUIService changed", I prefer a notification, > whereas for something like SigninTracker which has multiple possible states > (SigninSuccessful, SigninFailed, GaiaAuthenticationComplete) I prefer > listeners/delegates. > > The only two advantages of listeners/delegates that I know of are that > listeners/delegates let you encapsulate multiple API calls, and you get more > type safety (not really an issue for a simple boolean/typeless notification). > Are there other issues that make listeners preferable? There are a number of issues with notifications -they are a parallel API to places where we already have a well defined API (i.e. between content/chrome). even in one big module such as chrome, there are still different layers and this causes random parts to call each other. as more of the code, even in chrome, becomes componentized, global notifications get in the way of refactorings -type safety as you mention. we have had bugs where types of details change but not all callers are updated leading to crashes. this is impossible with listener interfaces -duplication with other interfaces. there are a number of places in the code that both notify using the notification service, and also have a listener interface. this is bad for all the obvious reasons -makes testing harder -more error prone in case a listener listens to all sources, instead of only what they want. this is impossible with a listener interface Not all these reasons may apply in this case, but they apply in the codebase with enough frequency which leads to my strong distaste for the notification system.
http://codereview.chromium.org/10699013/diff/9001/chrome/common/chrome_notifi... File chrome/common/chrome_notification_types.h (right): http://codereview.chromium.org/10699013/diff/9001/chrome/common/chrome_notifi... chrome/common/chrome_notification_types.h:875: NOTIFICATION_LOGIN_UI_CHANGED, On 2012/07/09 19:48:54, munjal wrote: > On 2012/07/09 19:38:13, John Abd-El-Malek wrote: > > On 2012/07/09 19:24:59, munjal wrote: > > > On 2012/07/09 17:35:53, John Abd-El-Malek wrote: > > > > On 2012/07/09 17:30:06, munjal wrote: > > > > > On 2012/07/04 04:25:21, John Abd-El-Malek wrote: > > > > > > On 2012/07/03 17:52:17, munjal wrote: > > > > > > > On 2012/07/03 05:52:27, John Abd-El-Malek wrote: > > > > > > > > who's listening to this notification? > > > > > > > > > > > > > > The listening code is coming up in the next patch. > > > > > > > > > > > > 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. > > > > > > > > > > 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. > > > > > > > > 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? > > > > > > 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. > > > > Given that LoginUIService is a profile keyed service, and profiles live until > > browser shutdown, I don't see the lifetime issue? > > > > > > > > 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. > > > > 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. This is a common pattern, and we have ObserverList precisely to solve this case. > > 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.
http://codereview.chromium.org/10699013/diff/9001/chrome/browser/ui/webui/sig... File chrome/browser/ui/webui/signin/login_ui_service.cc (right): http://codereview.chromium.org/10699013/diff/9001/chrome/browser/ui/webui/sig... chrome/browser/ui/webui/signin/login_ui_service.cc:52: void LoginUIService::ShowLoginDialogUI(Browser* browser) { On 2012/07/09 19:52:06, Andrew T Wilson wrote: > On 2012/07/03 17:52:17, munjal wrote: > > On 2012/07/03 17:34:09, Andrew T Wilson wrote: > > > This isn't displaying a dialog - it's displaying a popup. > > > > > > Does it make sense to just allow the caller to pass in the type of > > > WindowOpenDisposition, for added flexibility? > > > > > > Also, I'm not at all sure that this is so generally useful that we need to > > make > > > a common API for it. If it's only going to get called from one place, can we > > > move this functionality into the caller instead? > > > > I changed the name to ShowLoginUIPopup. I think it is better to not let the > > calelrs pass the windowOpenDisposition yet. > > > > I don't mind moving it to the caller. But I think this is a better place since > > the caller has to do the same stuff about focusing existing UI if present etc. > > and for that it integrates with LoginUIService anyway. > > Let's put this in the caller (take it out of this patch). In fact, given that > ShowLoginUI() is so simple now (all the logic has been moved out to > chrome::ShowSettingsSubPage()) I'm planning to remove that UI too. Done.
I am fine with changing this to using ObserverList and adding two methods to LoginUIService, AddObserver and RemoveObserver. Drew, what do you think?
I changed to using an observer list, and updated the tests. Please take another look.
http://codereview.chromium.org/10699013/diff/21004/chrome/browser/ui/webui/si... File chrome/browser/ui/webui/signin/login_ui_service.cc (right): http://codereview.chromium.org/10699013/diff/21004/chrome/browser/ui/webui/si... chrome/browser/ui/webui/signin/login_ui_service.cc:17: if (o) why do you need to handle someone calling add/remove observer with a null pointer? http://codereview.chromium.org/10699013/diff/21004/chrome/browser/ui/webui/si... File chrome/browser/ui/webui/signin/login_ui_service.h (right): http://codereview.chromium.org/10699013/diff/21004/chrome/browser/ui/webui/si... chrome/browser/ui/webui/signin/login_ui_service.h:49: void AddObserver(Observer* o); nit: Observer* observer http://codereview.chromium.org/10699013/diff/21004/chrome/browser/ui/webui/si... chrome/browser/ui/webui/signin/login_ui_service.h:54: // Fires chrome::NOTIFICATION_LOGIN_UI_CHANGED. nit: remove this comment and below
http://codereview.chromium.org/10699013/diff/21004/chrome/browser/ui/webui/si... File chrome/browser/ui/webui/signin/login_ui_service.cc (right): http://codereview.chromium.org/10699013/diff/21004/chrome/browser/ui/webui/si... chrome/browser/ui/webui/signin/login_ui_service.cc:17: if (o) On 2012/07/10 16:13:07, John Abd-El-Malek wrote: > why do you need to handle someone calling add/remove observer with a null > pointer? Done. http://codereview.chromium.org/10699013/diff/21004/chrome/browser/ui/webui/si... File chrome/browser/ui/webui/signin/login_ui_service.h (right): http://codereview.chromium.org/10699013/diff/21004/chrome/browser/ui/webui/si... chrome/browser/ui/webui/signin/login_ui_service.h:49: void AddObserver(Observer* o); On 2012/07/10 16:13:07, John Abd-El-Malek wrote: > nit: Observer* observer Done. http://codereview.chromium.org/10699013/diff/21004/chrome/browser/ui/webui/si... chrome/browser/ui/webui/signin/login_ui_service.h:54: // Fires chrome::NOTIFICATION_LOGIN_UI_CHANGED. On 2012/07/10 16:13:07, John Abd-El-Malek wrote: > nit: remove this comment and below Done.
lgtm
LGTM, with a few nits. http://codereview.chromium.org/10699013/diff/9003/chrome/browser/ui/webui/sig... File chrome/browser/ui/webui/signin/login_ui_service.cc (right): http://codereview.chromium.org/10699013/diff/9003/chrome/browser/ui/webui/sig... chrome/browser/ui/webui/signin/login_ui_service.cc:35: ui_ = NULL; I would suggest clearing ui_ here before notifying observers. That way, if an observer wants to call SetLoginUI for some reason, they can do so. http://codereview.chromium.org/10699013/diff/9003/chrome/browser/ui/webui/sig... File chrome/browser/ui/webui/signin/login_ui_service.h (right): http://codereview.chromium.org/10699013/diff/9003/chrome/browser/ui/webui/sig... chrome/browser/ui/webui/signin/login_ui_service.h:34: public: Style guide requires a virtual destructor for interface classes like this. http://codereview.chromium.org/10699013/diff/9003/chrome/browser/ui/webui/sig... File chrome/browser/ui/webui/signin/login_ui_service_unittest.cc (right): http://codereview.chromium.org/10699013/diff/9003/chrome/browser/ui/webui/sig... chrome/browser/ui/webui/signin/login_ui_service_unittest.cc:69: // If the right UI is closed, notification shoudl be fired. nit: shoudl->should
+estade for OWNERS http://codereview.chromium.org/10699013/diff/9003/chrome/browser/ui/webui/sig... File chrome/browser/ui/webui/signin/login_ui_service.cc (right): http://codereview.chromium.org/10699013/diff/9003/chrome/browser/ui/webui/sig... chrome/browser/ui/webui/signin/login_ui_service.cc:35: ui_ = NULL; On 2012/07/10 17:37:43, Andrew T Wilson wrote: > I would suggest clearing ui_ here before notifying observers. That way, if an > observer wants to call SetLoginUI for some reason, they can do so. Done. http://codereview.chromium.org/10699013/diff/9003/chrome/browser/ui/webui/sig... File chrome/browser/ui/webui/signin/login_ui_service.h (right): http://codereview.chromium.org/10699013/diff/9003/chrome/browser/ui/webui/sig... chrome/browser/ui/webui/signin/login_ui_service.h:34: public: On 2012/07/10 17:37:43, Andrew T Wilson wrote: > Style guide requires a virtual destructor for interface classes like this. Done. http://codereview.chromium.org/10699013/diff/9003/chrome/browser/ui/webui/sig... File chrome/browser/ui/webui/signin/login_ui_service_unittest.cc (right): http://codereview.chromium.org/10699013/diff/9003/chrome/browser/ui/webui/sig... chrome/browser/ui/webui/signin/login_ui_service_unittest.cc:69: // If the right UI is closed, notification shoudl be fired. On 2012/07/10 17:37:43, Andrew T Wilson wrote: > nit: shoudl->should Done.
http://codereview.chromium.org/10699013/diff/31002/chrome/browser/ui/webui/si... File chrome/browser/ui/webui/signin/login_ui_service.h (right): http://codereview.chromium.org/10699013/diff/31002/chrome/browser/ui/webui/si... chrome/browser/ui/webui/signin/login_ui_service.h:33: class Observer { nit: Document class. http://codereview.chromium.org/10699013/diff/31002/chrome/browser/ui/webui/si... chrome/browser/ui/webui/signin/login_ui_service.h:35: virtual ~Observer() { } This is an interface, right? The destructor should be protected. http://codereview.chromium.org/10699013/diff/31002/chrome/browser/ui/webui/si... chrome/browser/ui/webui/signin/login_ui_service.h:35: virtual ~Observer() { } s/{ }/{}/ http://codereview.chromium.org/10699013/diff/31002/chrome/browser/ui/webui/si... chrome/browser/ui/webui/signin/login_ui_service.h:37: virtual void OnLoginUIShown(LoginUI* ui) = 0; Optional nit: I'd add a newline between methods for readability. http://codereview.chromium.org/10699013/diff/31002/chrome/browser/ui/webui/si... chrome/browser/ui/webui/signin/login_ui_service.h:50: void AddObserver(Observer* observer); nit: Document |observer|. http://codereview.chromium.org/10699013/diff/31002/chrome/browser/ui/webui/si... chrome/browser/ui/webui/signin/login_ui_service.h:70: ObserverList<Observer> observer_list_; nit: Document member variable.
http://codereview.chromium.org/10699013/diff/31002/chrome/browser/ui/webui/si... File chrome/browser/ui/webui/signin/login_ui_service.h (right): http://codereview.chromium.org/10699013/diff/31002/chrome/browser/ui/webui/si... chrome/browser/ui/webui/signin/login_ui_service.h:33: class Observer { On 2012/07/10 19:25:22, James Hawkins wrote: > nit: Document class. Done. http://codereview.chromium.org/10699013/diff/31002/chrome/browser/ui/webui/si... chrome/browser/ui/webui/signin/login_ui_service.h:35: virtual ~Observer() { } On 2012/07/10 19:25:22, James Hawkins wrote: > This is an interface, right? The destructor should be protected. Done. http://codereview.chromium.org/10699013/diff/31002/chrome/browser/ui/webui/si... chrome/browser/ui/webui/signin/login_ui_service.h:35: virtual ~Observer() { } On 2012/07/10 19:25:22, James Hawkins wrote: > s/{ }/{}/ Done. http://codereview.chromium.org/10699013/diff/31002/chrome/browser/ui/webui/si... chrome/browser/ui/webui/signin/login_ui_service.h:37: virtual void OnLoginUIShown(LoginUI* ui) = 0; On 2012/07/10 19:25:22, James Hawkins wrote: > Optional nit: I'd add a newline between methods for readability. Done. http://codereview.chromium.org/10699013/diff/31002/chrome/browser/ui/webui/si... chrome/browser/ui/webui/signin/login_ui_service.h:50: void AddObserver(Observer* observer); On 2012/07/10 19:25:22, James Hawkins wrote: > nit: Document |observer|. Done. http://codereview.chromium.org/10699013/diff/31002/chrome/browser/ui/webui/si... chrome/browser/ui/webui/signin/login_ui_service.h:70: ObserverList<Observer> observer_list_; On 2012/07/10 19:25:22, James Hawkins wrote: > nit: Document member variable. Done.
Who is going to be using this new functionality? It's currently deadcode. http://codereview.chromium.org/10699013/diff/29003/chrome/browser/ui/webui/si... File chrome/browser/ui/webui/signin/login_ui_service.h (right): http://codereview.chromium.org/10699013/diff/29003/chrome/browser/ui/webui/si... chrome/browser/ui/webui/signin/login_ui_service.h:37: virtual void OnLoginUIShown(LoginUI* ui) = 0; nit: Document |ui|, e.g. can it ever be NULL or is it guaranteed to be non-NULL? http://codereview.chromium.org/10699013/diff/29003/chrome/browser/ui/webui/si... chrome/browser/ui/webui/signin/login_ui_service.h:54: // |observer| The observer to add or remove. nit: Must it be non-NULL? http://codereview.chromium.org/10699013/diff/29003/chrome/browser/ui/webui/si... File chrome/browser/ui/webui/signin/login_ui_service_unittest.cc (right): http://codereview.chromium.org/10699013/diff/29003/chrome/browser/ui/webui/si... chrome/browser/ui/webui/signin/login_ui_service_unittest.cc:58: TEST_F(LoginUIServiceTest, LoginUIChangedNotification) { nit: Document specifically what the test is testing. http://codereview.chromium.org/10699013/diff/29003/chrome/browser/ui/webui/si... chrome/browser/ui/webui/signin/login_ui_service_unittest.cc:64: // If a different UI is closed than the one that was shown, no Optional nit: Some whitespace around chunks of tests would help readability.
http://codereview.chromium.org/10699013/diff/29003/chrome/browser/ui/webui/si... File chrome/browser/ui/webui/signin/login_ui_service.h (right): http://codereview.chromium.org/10699013/diff/29003/chrome/browser/ui/webui/si... chrome/browser/ui/webui/signin/login_ui_service.h:37: virtual void OnLoginUIShown(LoginUI* ui) = 0; On 2012/07/10 20:41:15, James Hawkins wrote: > nit: Document |ui|, e.g. can it ever be NULL or is it guaranteed to be non-NULL? Done. http://codereview.chromium.org/10699013/diff/29003/chrome/browser/ui/webui/si... chrome/browser/ui/webui/signin/login_ui_service.h:54: // |observer| The observer to add or remove. On 2012/07/10 20:41:15, James Hawkins wrote: > nit: Must it be non-NULL? Done. http://codereview.chromium.org/10699013/diff/29003/chrome/browser/ui/webui/si... File chrome/browser/ui/webui/signin/login_ui_service_unittest.cc (right): http://codereview.chromium.org/10699013/diff/29003/chrome/browser/ui/webui/si... chrome/browser/ui/webui/signin/login_ui_service_unittest.cc:58: TEST_F(LoginUIServiceTest, LoginUIChangedNotification) { On 2012/07/10 20:41:15, James Hawkins wrote: > nit: Document specifically what the test is testing. Done. http://codereview.chromium.org/10699013/diff/29003/chrome/browser/ui/webui/si... chrome/browser/ui/webui/signin/login_ui_service_unittest.cc:64: // If a different UI is closed than the one that was shown, no On 2012/07/10 20:41:15, James Hawkins wrote: > Optional nit: Some whitespace around chunks of tests would help readability. Done.
LGTM In general I'm not a fan of CLs that add code w/out usage, but I'll take your word you have a CL w/ usage you'll put up later today :-)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/munjal@chromium.org/10699013/29005
nti: update the change description
Try job failure for 10699013-29005 (retry) on win for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/munjal@chromium.org/10699013/32005
Change committed as 146143 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
