I just helped out a provider who was stung by the fact that we silently refused to open a chat window when no user was logged in. This restriction seems somewhat arbitrary and doesn't offer any protection against abuse of the social API. I see no good reason for this restriction and there may even be use-cases to allow it (eg, imagine a provider allowing a chat session to their support personnel when the user can't log in)

IMO there is only one place that haveLoggedInUser makes sense, that is in showProfile. We should just check for provider.profile rather than trying to have a loggedin api.

A fly in the ointment here is that we currently close all chats when the user logs out, which is something we need to maintain. Doing this sanely means our API would need to insist that we only ever get exactly 1 notification from the provider when the user is logged out (otherwise, we would end up in the situation where chats were open in the "not logged in" case, and if the provider later tells us "no one is still logged in" we would end up closing all those chats.) Or we'd end up with far more complex state management for something where there isn't an identified realistic use-case. So I think I'll close this as INVALID over the next few days unless someone objects - Shane, what do you think?

(In reply to Mark Hammond (:markh) from comment #2) > (otherwise, we > would end up in the situation where chats were open in the "not logged in" > case, and if the provider later tells us "no one is still logged in" we > would end up closing all those chats.) Or we'd end up with far more complex > state management ... Actually, a very similar problem could exist today - see bug 850969.

I think we can do two things here. 1. if a provider doesn't use login, we allow chat windows to open anyway and we close them on provider change. 2. if a provider does use login, we should close chat windows if we receive a new user-profile where the username does not match the previous profile.

As long as we maintain user-profile api I'm now thinking that if we get an empty profile sent, we close all chat windows (for that provider) as if the user logged out, regardless of whether there was a previous login.

(In reply to Shane Caraveo (:mixedpuppy) from comment #5) > As long as we maintain user-profile api I'm now thinking that if we get an > empty profile sent, we close all chat windows (for that provider) as if the > user logged out, regardless of whether there was a previous login. I'm not sure how this addresses comment 2 - ie, if there is a chat window opened when noone is logged in, and the provider again happens to mention that noone is still logged in, we don't want to close the open chats. I'd have thought something like comment 4 makes sense - only close chats when the login state changes, using the userid as a 'key'

(In reply to Mark Hammond (:markh) from comment #6) > I'd have thought something like comment 4 makes sense - only close chats > when the login state changes, using the userid as a 'key' yes, that would be better.

moved login restriction removal from patch in bug 891221 to here.

Comment on attachment 777872 [details] [diff] [review] nologin restriction Review of attachment 777872 [details] [diff] [review]: ----------------------------------------------------------------- The earlier comments in this bug don't seem to have been implemented at all? ::: browser/base/content/test/social/browser_social_chatwindow.js @@ +439,5 @@ > openChat(function() { > ok(!window.SocialChatBar.hasChats, "first window has no chats"); > ok(secondWindow.SocialChatBar.hasChats, "second window has a chat"); > secondWindow.close(); > + port.close(); do the tests really fail if this isn't done here? ::: browser/base/content/test/social/browser_social_toolbar.js @@ +123,5 @@ > } catch(e) {} > let numIcons = Object.keys(Social.provider.ambientNotificationIcons).length; > ok(numIcons == 3, "prevent adding more than 3 ambient notification icons"); > > + let mButton = document.getElementById("social-mark-button"); does the change to browser-social really cause this pre-existing test bug to be hit? It seems quite unrelated.

(In reply to Mark Hammond (:markh) from comment #10) > Comment on attachment 777872 [details] [diff] [review] > nologin restriction > > Review of attachment 777872 [details] [diff] [review]: > ----------------------------------------------------------------- > > The earlier comments in this bug don't seem to have been implemented at all? The only item not checked is if the username changed or not, otherwise comment #4 is implemented. There isn't a clean way to do that right now, so I punted in order to think about it more. > ::: browser/base/content/test/social/browser_social_chatwindow.js > @@ +439,5 @@ > > openChat(function() { > > ok(!window.SocialChatBar.hasChats, "first window has no chats"); > > ok(secondWindow.SocialChatBar.hasChats, "second window has a chat"); > > secondWindow.close(); > > + port.close(); > > do the tests really fail if this isn't done here? yes, the onmessage was staying around long enough to cause spurious messages/errors in the next test. > ::: browser/base/content/test/social/browser_social_toolbar.js > @@ +123,5 @@ > > } catch(e) {} > > let numIcons = Object.keys(Social.provider.ambientNotificationIcons).length; > > ok(numIcons == 3, "prevent adding more than 3 ambient notification icons"); > > > > + let mButton = document.getElementById("social-mark-button"); > > does the change to browser-social really cause this pre-existing test bug to > be hit? It seems quite unrelated. yes, and I looked further into this, doesn't make sense at all (outside of timing, ambient buttons added prior to the wait), which tells me it could end up as an intermittent at some point. I'm happy to move this to a separate bug and get it landed.

toolbar issue will move to another bug now only closes chat windows if the userName changes rather than on any profile change (e.g. the user could change their image)

Comment on attachment 781848 [details] [diff] [review] nologin Review of attachment 781848 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/browser-social.js @@ +398,5 @@ > if (dwu.isHandlingUserInput) > this.chatbar.focus(); > return true; > }, > + closeForOrigin: function(origin) { it still doesn't make sense to me that this code, living in every window, is also doing the enumeration of top-level windows. The observer that drives this is sent in SocialService, which inturn already has a reference to MozSocialAPI, which inturn already has "open chat" functions exported. So why not just drop the observer completely, and have SocialService call a new function exported from MozSocialAPI for "close chat" capabilities?

based on irc discussion

Comment on attachment 782317 [details] [diff] [review] nologin Review of attachment 782317 [details] [diff] [review]: ----------------------------------------------------------------- thanks! ::: browser/base/content/test/social/browser_chat_tearoff.js @@ +80,2 @@ > domwindow.removeEventListener("load", _load, false); > + trailing space ::: toolkit/components/social/SocialService.jsm @@ +838,5 @@ > // Called by the workerAPI to update our profile information. > updateUserProfile: function(profile) { > if (!profile) > profile = {}; > + let accountChanged = !this.profile || this.profile.userName != profile.userName; We might as well handle the logged-out case better too - I think simply: .. (!this.profile != !profile) || (this.profile.userName != profile.userName); will work?

(In reply to Mark Hammond (:markh) from comment #15) > ::: toolkit/components/social/SocialService.jsm > @@ +838,5 @@ > > // Called by the workerAPI to update our profile information. > > updateUserProfile: function(profile) { > > if (!profile) > > profile = {}; > > + let accountChanged = !this.profile || this.profile.userName != profile.userName; > > We might as well handle the logged-out case better too - I think simply: > > .. (!this.profile != !profile) || (this.profile.userName != > profile.userName); > > will work? !profile will always be false in this case. so that really becomes "if (this.profile || this.profile.userName != profile.userName);" which wont work if this.profile is undefined. I'm not clear what you're trying to catch that I'm already not catching.

Yeah, I didn't look closely enough at the context. What I was trying to catch was calling with a null profile when this.profile is already null (but yeah, this.profile and profile are never null given the line above...)

(but I guess Object.keys(profile).length could work?)

update for bitrot

patch to land independently of frameworker changes per irc discussion. just asking for a quick re-review. https://tbpl.mozilla.org/?tree=Try&rev=13f8f04eb4de

Discovered a bug with closeAllChatWindows while doing some manual testing. This patch fixes the problem (caused by using element.children incorrectly) as modifies the test to watch for that in the future. https://tbpl.mozilla.org/?tree=Try&rev=4af652404700

Comment on attachment 790459 [details] [diff] [review] nologin Review of attachment 790459 [details] [diff] [review]: ----------------------------------------------------------------- LGTM ::: browser/base/content/test/social/browser_chat_tearoff.js @@ +80,2 @@ > domwindow.removeEventListener("load", _load, false); > + whitespace nit @@ +173,5 @@ > + next(); > + }, false); > + > + is(doc.documentElement.getAttribute("windowtype"), "Social:Chat", "Social:Chat window opened"); > + is(doc.location.href, "chrome://browser/content/chatWindow.xul", "Should have seen the right window open"); this check is redundant given the check at the top of the listener

updated with comments https://hg.mozilla.org/integration/fx-team/rev/c211aa7adf48