enable opening chats from worker

RESOLVED FIXED in Firefox 17

Status

()

Firefox
SocialAPI
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

Tracking

unspecified
Firefox 17
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Fx17])

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

5 years ago
we'll allow new chats to be opened in minimized mode.
(Assignee)

Updated

5 years ago
Assignee: nobody → mixedpuppy
(Assignee)

Comment 1

5 years ago
Created attachment 654034 [details] [diff] [review]
worker chat api

feedback request for worker api that opens minimized chat.  as well, this patch prevents reopening the same chat twice, relying on unique urls per chat.
Attachment #654034 - Flags: feedback?(jaws)
Attachment #654034 - Flags: feedback?(felipc)
Comment on attachment 654034 [details] [diff] [review]
worker chat api

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

If an openChat call happens to a minimized chat, will we want it to un-minimize it?

::: browser/base/content/socialchat.xml
@@ +134,5 @@
>  
>        <field name="selectedChat"/>
>  
>        <field name="menuitemMap">new WeakMap()</field>
> +      <field name="chatUrlMap">new Object();</field>

you can use a Map() instead of an object, it has the same API as a WeakMap but you can use strings as keys (and it's not automatic weak refs, of course)

if you name the variable chatboxForURL then it will work out pretty nice:
  chatboxForURL.set(url, cb);
  chatboxForURL.get(url)

@@ +263,5 @@
> +          if (this.chatUrlMap[aURL]) {
> +            let cb = this.chatUrlMap[aURL];
> +            // ensure this chatbox is visible and focused
> +            if (this.selectedChat != cb)
> +              this.selectedChat = cb;

does this actually give focus to the content, or just style it so? Maybe you first need the showChat call, and then call cb.focus()? selectedChat should automatically update then

I don't know if that will be enough to focus the content inside it (a textbox for example), but I believe it will

@@ +274,5 @@
>            this.appendChild(cb);
>            cb.init(aProvider, aURL, aCallback);
> +          this.chatUrlMap[aURL] = Cu.getWeakReference(cb);
> +          if (aMode == "minimized")
> +            cb.minimized = true;

does cb.minimized work (together with the toggle() function), or do you need cb.setAttribute("minimized")?

you should set the property before appending it to the tree so there's no CSS restyling needed

is socialFrameShow still intended to fire when opening the minimized chat? Or only when it is unminimized?
Attachment #654034 - Flags: feedback?(felipc) → feedback+

Comment 3

5 years ago
I'm not convinced we want this to be the default option. If we can give the social service the choice, I think that would be ideal. Also, and for a later but, we might want this to be user controlled.
(Assignee)

Comment 4

5 years ago
(In reply to Asa Dotzler [:asa] from comment #3)
> I'm not convinced we want this to be the default option. If we can give the
> social service the choice, I think that would be ideal. Also, and for a
> later but, we might want this to be user controlled.

Asa, I'm assuming you're commenting on the minimized option.  This patch is not defining that as the default.  Default is having the chat open.  As discussed, if the worker opens it, we will open in minimized mode.

Comment 5

5 years ago
OK. I think I misunderstood this bug. In what case(s) does the worker open the chat?
(Assignee)

Comment 6

5 years ago
(In reply to Asa Dotzler [:asa] from comment #5)
> OK. I think I misunderstood this bug. In what case(s) does the worker open
> the chat?

The worker would open chat if the sidebar is not visible, or, in a multi provider world, the provider is not the currently selected provider.
Attachment #654034 - Flags: feedback?(jaws)
(Assignee)

Comment 7

5 years ago
(In reply to :Felipe Gomes from comment #2)
> Comment on attachment 654034 [details] [diff] [review]

> @@ +263,5 @@
> > +          if (this.chatUrlMap[aURL]) {
> > +            let cb = this.chatUrlMap[aURL];
> > +            // ensure this chatbox is visible and focused
> > +            if (this.selectedChat != cb)
> > +              this.selectedChat = cb;
> 
> does this actually give focus to the content, or just style it so? Maybe you
> first need the showChat call, and then call cb.focus()? selectedChat should
> automatically update then

selectedChat is really just to track the chat window we do not want to collapse into the nub if there is an overflow.  We're intentionally not focusing the content in any case.

> does cb.minimized work (together with the toggle() function), or do you need
> cb.setAttribute("minimized")?

they work together.

> is socialFrameShow still intended to fire when opening the minimized chat?
> Or only when it is unminimized?

yes.
(Assignee)

Comment 8

5 years ago
Created attachment 655223 [details] [diff] [review]
worker chat api

updated from comments, taking the opportunity to correct the insertion of chat windows as well.
Attachment #654034 - Attachment is obsolete: true
Attachment #655223 - Flags: feedback?(jaws)
Attachment #655223 - Flags: feedback?(felipc)
(Assignee)

Comment 9

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=2894ea6571f1
Comment on attachment 655223 [details] [diff] [review]
worker chat api

> if (cb) {
>  // ensure this chatbox is visible and focused
>  if (this.selectedChat != cb)
>    this.selectedChat = cb;
>  if (cb.collapsed)
>    this.showChat(cb);
>  return;

This part still feels a bit weird to me as in not focusing the chat when opening it. But it seesm this is a conscious decision (although I believe we might change that later).  In any case, if you keep this current behavior, update the comment above that says "ensure is visible and focused" to something that says the truth :)


Everything else looks good to me. You asked for feedback so I don't know if you still plan to do more things in this patch. I'll mark r+ with the comment above updated but feel free to re-request review if there are more iterations needed.
Attachment #655223 - Flags: feedback?(felipc) → review+
(Assignee)

Comment 11

5 years ago
(In reply to :Felipe Gomes from comment #10)
> Comment on attachment 655223 [details] [diff] [review]
> worker chat api
> 
> > if (cb) {
> >  // ensure this chatbox is visible and focused
> >  if (this.selectedChat != cb)
> >    this.selectedChat = cb;
> >  if (cb.collapsed)
> >    this.showChat(cb);
> >  return;
> 
> This part still feels a bit weird to me as in not focusing the chat when
> opening it. But it seesm this is a conscious decision (although I believe we
> might change that later).  In any case, if you keep this current behavior,
> update the comment above that says "ensure is visible and focused" to
> something that says the truth :)

heh, yeah, I was thinking to focus it before, but since we will be allowing a background process to open chats I think stealing focus from, for example a wiki editor while you're typing, would be a bit rude.

I'm figuring we'll be tweaking a lot around chat after we start to get real user feedback.
(Assignee)

Comment 12

5 years ago
Created attachment 655244 [details] [diff] [review]
patch with updated comment

changed the comment on focus
Attachment #655223 - Attachment is obsolete: true
Attachment #655223 - Flags: feedback?(jaws)
Attachment #655244 - Flags: review?(jaws)
Comment on attachment 655244 [details] [diff] [review]
patch with updated comment

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

::: browser/base/content/test/social_sidebar.html
@@ +13,5 @@
>                break;
>              case "test-chatbox-open":
> +              var url = "social_chat.html";
> +              if (e.data.data && e.data.data.id) {
> +                url = url + "?id="+e.data.id;

url += "?id=" + e.data.id;

::: toolkit/components/social/WorkerAPI.jsm
@@ +71,5 @@
>                                data: results});
>      },
> +    'social.request-chat': function(data) {
> +      let xulWindow = Services.wm.getMostRecentWindow("navigator:browser").getTopWin();
> +      openChatWindow(xulWindow, this._provider, data, null, "minimized");

Hm, I just thought about how the chats are only going to show up in one browser window. We should be keeping the same chat windows in all navigator:browser windows so users don't have to search for the window that their chat was in. I filed bug 785587 for this.
Attachment #655244 - Flags: review?(jaws) → review+
Oh I forgot to mention in my review feedback, the Map is storing weak references (Cu.getWeakReference), and according to the documentation we should be using .get() on the weak reference before using it. Further, we should be checking for a valid parentNode to make sure that the element is still in the DOM.

Can you make these changes?
(Assignee)

Comment 15

5 years ago
Created attachment 655457 [details] [diff] [review]
worker chat api

updated with comments, fixed tests and added one more
Attachment #655244 - Attachment is obsolete: true
Attachment #655457 - Flags: review?(jaws)
Attachment #655457 - Flags: review?(gavin.sharp)
(Assignee)

Comment 16

5 years ago
new try push https://tbpl.mozilla.org/?tree=Try&rev=f537ba476b5f
(Assignee)

Comment 17

5 years ago
Created attachment 655472 [details] [diff] [review]
worker chat api

fixes unrelated social test that showed up breaking at this point
Attachment #655457 - Attachment is obsolete: true
Attachment #655457 - Flags: review?(jaws)
Attachment #655457 - Flags: review?(gavin.sharp)
Attachment #655472 - Flags: review?(jaws)
(Assignee)

Comment 18

5 years ago
new try https://tbpl.mozilla.org/?tree=Try&rev=b71f2afe3741
Attachment #655472 - Flags: review?(jaws) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/5dbb817a449c
Flags: in-testsuite+
OS: Mac OS X → All
Hardware: x86 → All
https://hg.mozilla.org/mozilla-central/rev/5dbb817a449c
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 17
You need to log in before you can comment on or make changes to this bug.