chat support from multiple providers

RESOLVED FIXED in Firefox 26

Status

()

Firefox
SocialAPI
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

Tracking

23 Branch
Firefox 26
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Updated

5 years ago
Blocks: 889427
(Assignee)

Updated

5 years ago
Blocks: 891223
Created attachment 772586 [details] [diff] [review]
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.
(Assignee)

Comment 2

5 years ago
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.
(Assignee)

Comment 3

5 years ago
Created attachment 776331 [details] [diff] [review]
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
Attachment #772586 - Attachment is obsolete: true
Attachment #776331 - Flags: feedback?(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?
Attachment #776331 - Flags: feedback?(mhammond) → feedback+
(Assignee)

Updated

5 years ago
Depends on: 840870
(Assignee)

Comment 5

5 years ago
Created attachment 777874 [details] [diff] [review]
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
Attachment #776331 - Attachment is obsolete: true
Attachment #777874 - Flags: feedback?(mhammond)

Updated

5 years ago
Attachment #777874 - Flags: feedback?(mhammond) → feedback+
(Assignee)

Comment 6

5 years ago
Created attachment 789202 [details] [diff] [review]
chat for multiple providers

update for bitrot
Attachment #777874 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Blocks: 891225
(Assignee)

Updated

5 years ago
Depends on: 891216
No longer depends on: 840870
(Assignee)

Updated

5 years ago
Depends on: 905297
Flags: needinfo?(mhammond)
(Assignee)

Comment 7

5 years ago
Created attachment 790530 [details] [diff] [review]
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.
Attachment #789202 - Attachment is obsolete: true
Attachment #790530 - Flags: review?(mhammond)
Flags: needinfo?(mhammond)
(Assignee)

Comment 8

5 years ago
(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.
(Assignee)

Comment 9

5 years ago
(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
Depends on: 904521
No longer depends on: 905297
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
Attachment #790530 - Flags: review?(mhammond) → review-
(Assignee)

Comment 11

5 years ago
Created attachment 792445 [details] [diff] [review]
tests for mutliprovider chats

feedback implemented.  note that test3.example.com doesn't work, so just making names match better to domains.
Assignee: nobody → mixedpuppy
Attachment #790530 - Attachment is obsolete: true
Attachment #792445 - Flags: review?(mhammond)
Comment on attachment 792445 [details] [diff] [review]
tests for mutliprovider chats

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

Thanks!
Attachment #792445 - Flags: review?(mhammond) → review+
https://hg.mozilla.org/mozilla-central/rev/d0064759d817
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
You need to log in before you can comment on or make changes to this bug.