891221 - chat support from multiple providers

Status: RESOLVED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[Shane Caraveo (:mixedpuppy)]

We need to examine supporting incomming calls/chats from multiple providers.  The only obvious issue is SocialChatBar.isAvailable which is relying on the currentProvider having a loggedin user.

Comment 1

[Florian Quèze [:florian]]

Attachment: WIP

(In reply to Shane Caraveo (:mixedpuppy) from comment #0)

> The only obvious issue is SocialChatBar.isAvailable which is relying on the
> currentProvider having a loggedin user.

The attached WIP removes this restriction. It may be more related to bug 840870 though.

Comment 2

[Shane Caraveo (:mixedpuppy)]

I think we might have to change haveLoggedInUser to either take profile as an argument, or just do the check locally.  afaik SocialChatBar.isAvailable could be private to that class (other than for the test).

There are also other situations that will need to change for this, I just noticed the provider-set notification handler closes all chat windows.  I don't think that makes sense now.

Comment 3

[Shane Caraveo (:mixedpuppy)]

Attachment: WIP chat for multiple providers

includes work for bug 840870 which could be split out.  

also contains a fix to the toolbar test, which is a little confounding as that test should not have been passing before, and somehow these changes triggered the failure of it.  as well, the call to setAmbientNotification removes the existing button and adds a new button.  A similar fix for the wait condition was done a short while ago in browser/base/content/test/social/browser_social_mozSocial_API.js

Comment 4

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 776331 [details] [diff] [review]
WIP chat for multiple providers

Review of attachment 776331 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser-social.js
@@ -99,5 @@
>            this._updateActiveUI();
>            this._updateMenuItems();
>  
>            SocialFlyout.unload();
> -          SocialChatBar.closeWindows();

I think this means closeWindows can be deleted?

@@ +374,5 @@
>    },
>    // Whether the chatbar is available for this window.  Note that in full-screen
>    // mode chats are available, but not shown.
>    get isAvailable() {
> +    return SocialUI.enabled;

I think that if we want to make this change, we have to do it in the context of bug 840870 (ie, that bug should block this one)

@@ +391,5 @@
>      if (dwu.isHandlingUserInput)
>        this.chatbar.focus();
>      return true;
>    },
> +  closeForOrigin: function(origin) {

it seems wrong that this method is closing all detached windows (which aren't actually associated with this |window|, right?), plus all chat windows that are associated.

Not sure what to do about that though - adding it to socialchat.xml also makes no real sense - maybe a method in MozSocialAPI.jsm - it already has openChatWindow()?

@@ +399,5 @@
> +      let winOrigin = win.document.getElementByID("chatter").getAttribute("origin");
> +      if (origin == winOrigin)
> +        win.close();
> +    }
> +    let chats = [c for (c of this.chatbar.children) if (c.getAttribute("origin") == origin)];

[c.close() for (c ....]; and you can drop the next 2 lines.

::: browser/base/content/test/social/browser_social_chatwindow.js
@@ +1,4 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  

I can't see an existing test here that changes the profile for a provider - so it looks like we need such a test so we can exercise closeForOrigin.

@@ +473,5 @@
> +      // an exception is thrown if same-origin worker/chaturl fails in openChatWindow
> +      let chatUrl = provider.origin + "/browser/browser/base/content/test/social/social_chat.html";
> +      gURLsNotRemembered.push(chatUrl);
> +      port.postMessage({topic: "test-worker-chat", data: chatUrl});
> +    }

need port.close()

@@ +474,5 @@
> +      let chatUrl = provider.origin + "/browser/browser/base/content/test/social/social_chat.html";
> +      gURLsNotRemembered.push(chatUrl);
> +      port.postMessage({topic: "test-worker-chat", data: chatUrl});
> +    }
> +    waitForCondition(function() chats.children.length == 3,

== Social.provider.length sounds better?

Comment 5

[Shane Caraveo (:mixedpuppy)]

Attachment: WIP chat for multiple providers

feedback implemented, added an extra test for logout of one provider when multiple have open chats (the last test in file also tests the profile change closing chats).  moved login restriction to bug 840870

Comment 6

[Shane Caraveo (:mixedpuppy)]

Attachment: chat for multiple providers

update for bitrot

Comment 7

[Shane Caraveo (:mixedpuppy)]

Attachment: tests for mutliprovider chats

between the patches in bug 840870 and bug 905297, we support chats from more than one provider at a time.  This patch adds a test for it.

Comment 8

[Shane Caraveo (:mixedpuppy)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #7)

> between the patches in bug 840870 and bug 905297, we support chats from more
> than one provider at a time.  This patch adds a test for it.

Actually, I meant bug 891216 and bug 905297.

Comment 9

[Shane Caraveo (:mixedpuppy)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #8)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #7)
> 
> > between the patches in bug 840870 and bug 905297, we support chats from more
> > than one provider at a time.  This patch adds a test for it.
> 
> Actually, I meant bug 891216 and bug 905297.

sigh, I obviously have to step away from bugzilla for the day.

I *really* meant bug 891216 and bug 904521

Comment 10

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 790530 [details] [diff] [review]
tests for mutliprovider chats

Review of attachment 790530 [details] [diff] [review]:
-----------------------------------------------------------------

I think this test needs more work.

::: browser/base/content/test/social/browser_social_chatwindow.js
@@ +24,5 @@
> +    origin: "https://test2.example.com",
> +    sidebarURL: "https://test2.example.com/browser/browser/base/content/test/social/social_sidebar.html?provider2",
> +    workerURL: "https://test2.example.com/browser/browser/base/content/test/social/social_worker.js",
> +    iconURL: "chrome://branding/content/icon48.png"
> +  }

can we make the "1" and "2"s consistent here? I initially thought there was a bug (eg, provider 2 can use test2.example.com and ?provider2, etc)

@@ -51,5 @@
>              let content = chats.selectedChat.content;
>              content.addEventListener("unload", function chatUnload() {
>                content.removeEventListener("unload", chatUnload, true);
>                ok(true, "got chatbox unload on close");
> -              port.close();

why the removals of these?

@@ +455,5 @@
>      });
>    },
> +  testMultipleProviderChat: function(next) {
> +    function setWorkerMode(multiple, cb) {
> +      // SocialService.enabled does not propogate upwards to Social.enabled

but the reverse is true, right?  ie, setting Social.enabled does the right thing IIRC.  If it doesn't, we have deeper problems we need to sort out.  Ditto for a few other places further down.

@@ +458,5 @@
> +    function setWorkerMode(multiple, cb) {
> +      // SocialService.enabled does not propogate upwards to Social.enabled
> +      Social.enabled = false;
> +      SocialService.enabled = false;
> +      waitForCondition(function() !Social.enabled, function() {

this wait doesn't look correct - haven't we just set it above?

@@ +465,5 @@
> +        else
> +          Services.prefs.clearUserPref("social.allowMultipleWorkers");
> +        SocialService.enabled = true;
> +        Social.enabled = true;
> +        waitForCondition(function() Social.enabled && Social.provider.getWorkerPort(),

this looks wrong too - once social is enabled, isn't the provider guaranteed to return a port?

@@ +491,5 @@
> +          let port = provider.getWorkerPort();
> +          port.postMessage({topic: "test-logout"});
> +          waitForCondition(function() chats.children.length == Social.providers.length - 1,
> +            function() {
> +              ok(chats.children.length == Social.providers.length - 1, "one chat window closed")

redundant given waitForCondition already checks that

Comment 11

[Shane Caraveo (:mixedpuppy)]

Attachment: tests for mutliprovider chats

feedback implemented.  note that test3.example.com doesn't work, so just making names match better to domains.

Comment 12

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 792445 [details] [diff] [review]
tests for mutliprovider chats

Review of attachment 792445 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

Comment 13

[Shane Caraveo (:mixedpuppy)]

https://tbpl.mozilla.org/?tree=Try&rev=5e6bc66ffe81

Comment 14

[Shane Caraveo (:mixedpuppy)]

https://hg.mozilla.org/integration/fx-team/rev/d0064759d817

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d0064759d817