Last Comment Bug 784535 - enable opening chats from worker
: enable opening chats from worker
Status: RESOLVED FIXED
[Fx17]
:
Product: Firefox
Classification: Client Software
Component: SocialAPI (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 17
Assigned To: Shane Caraveo (:mixedpuppy)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-21 15:37 PDT by Shane Caraveo (:mixedpuppy)
Modified: 2013-12-27 14:25 PST (History)
4 users (show)
markh: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
worker chat api (9.54 KB, patch)
2012-08-21 17:38 PDT, Shane Caraveo (:mixedpuppy)
felipc: feedback+
Details | Diff | Splinter Review
worker chat api (9.54 KB, patch)
2012-08-24 16:49 PDT, Shane Caraveo (:mixedpuppy)
felipc: review+
Details | Diff | Splinter Review
patch with updated comment (10.03 KB, patch)
2012-08-24 18:49 PDT, Shane Caraveo (:mixedpuppy)
jaws: review+
Details | Diff | Splinter Review
worker chat api (12.39 KB, patch)
2012-08-26 14:04 PDT, Shane Caraveo (:mixedpuppy)
no flags Details | Diff | Splinter Review
worker chat api (13.26 KB, patch)
2012-08-26 16:54 PDT, Shane Caraveo (:mixedpuppy)
jaws: review+
Details | Diff | Splinter Review

Description Shane Caraveo (:mixedpuppy) 2012-08-21 15:37:47 PDT
we'll allow new chats to be opened in minimized mode.
Comment 1 Shane Caraveo (:mixedpuppy) 2012-08-21 17:38:35 PDT
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.
Comment 2 :Felipe Gomes (needinfo me!) 2012-08-23 15:14:24 PDT
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?
Comment 3 Asa Dotzler [:asa] 2012-08-23 15:37:06 PDT
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.
Comment 4 Shane Caraveo (:mixedpuppy) 2012-08-23 15:45:31 PDT
(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 Asa Dotzler [:asa] 2012-08-23 16:11:41 PDT
OK. I think I misunderstood this bug. In what case(s) does the worker open the chat?
Comment 6 Shane Caraveo (:mixedpuppy) 2012-08-23 16:30:33 PDT
(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.
Comment 7 Shane Caraveo (:mixedpuppy) 2012-08-24 16:43:24 PDT
(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.
Comment 8 Shane Caraveo (:mixedpuppy) 2012-08-24 16:49:04 PDT
Created attachment 655223 [details] [diff] [review]
worker chat api

updated from comments, taking the opportunity to correct the insertion of chat windows as well.
Comment 9 Shane Caraveo (:mixedpuppy) 2012-08-24 17:31:54 PDT
https://tbpl.mozilla.org/?tree=Try&rev=2894ea6571f1
Comment 10 :Felipe Gomes (needinfo me!) 2012-08-24 18:24:25 PDT
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.
Comment 11 Shane Caraveo (:mixedpuppy) 2012-08-24 18:38:54 PDT
(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.
Comment 12 Shane Caraveo (:mixedpuppy) 2012-08-24 18:49:40 PDT
Created attachment 655244 [details] [diff] [review]
patch with updated comment

changed the comment on focus
Comment 13 Jared Wein [:jaws] (please needinfo? me) 2012-08-24 22:31:38 PDT
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.
Comment 14 Jared Wein [:jaws] (please needinfo? me) 2012-08-24 22:33:26 PDT
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?
Comment 15 Shane Caraveo (:mixedpuppy) 2012-08-26 14:04:45 PDT
Created attachment 655457 [details] [diff] [review]
worker chat api

updated with comments, fixed tests and added one more
Comment 16 Shane Caraveo (:mixedpuppy) 2012-08-26 14:06:42 PDT
new try push https://tbpl.mozilla.org/?tree=Try&rev=f537ba476b5f
Comment 17 Shane Caraveo (:mixedpuppy) 2012-08-26 16:54:21 PDT
Created attachment 655472 [details] [diff] [review]
worker chat api

fixes unrelated social test that showed up breaking at this point
Comment 18 Shane Caraveo (:mixedpuppy) 2012-08-26 16:54:48 PDT
new try https://tbpl.mozilla.org/?tree=Try&rev=b71f2afe3741

Note You need to log in before you can comment on or make changes to this bug.