1002914 - Refactor chat window so it can be used by social and loop

Status: VERIFIED FIXED

(Hello (Loop) :: Client, defect, P1)

Description

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

We want the loop client to be able to use the chat window without going via the social API.

Comment 1

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

Attachment: refactor-social.patch

This is a WIP, but worth getting early feedback on.

The notable, and possibly controversial, changes:

* We no longer remove the social error listeners as I couldn't work out a clean way to implement this.  Best I can tell this doesn't cause any problems - the iframe itself gies away (and thus the iframe.socialErrorListener attribute is removed), and the progress listener itself uses weak references.  The tests report no leaks.

* socialchat.xml uses promises instead of callbacks.  This has 2 main advantages:
** No need to determine if the callbacks have already fired and treat that as a special case - once the promise gets resolved, it remains resolved.
** There are now 2 promises - chatCreated and chatLoaded.  Implementing this with callbacks was going to get messy.

The (hopefully) less controversial changes:

* The social code uses these promises to add the social listener.  Thus the loop code will not get social error listeners (but obviously still have scope to do whatever ends up necessary in the loop context)

* There is a new test to ensure social listeners survive across detaching a window.

Comment 2

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8414222 [details] [diff] [review]
refactor-social.patch

overall I think this looks really good.

- I'm thinking we should just get rid of the chatactivity event entirely, use favicon and title updates to change the icon/text, and dont worry about the titlebar flashing (if that is even still there).
- is there any chance that loop will have error conditions that need an error page here?

Comment 3

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

hrmph - one issue with this patch is that even though the promise is resolved in DOMContentLoaded, there is an event-loop spin before the consumer of the promise has the .then() handler called.  This means it is typically *after* DOMContentLoaded has completed and scripts have started executing.  Shane, do you envisage this being a problem for social providers?

Comment 4

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

and, the patch still need love for reattaching a docked chat - it calls into social to find a window to use for chats.  That logic includes checks to see if social is enabled.  I'm not sure on the best way to resolve that.

Comment 5

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

(In reply to Mark Hammond [:markh] from comment #3)
> hrmph - one issue with this patch is that even though the promise is
> resolved in DOMContentLoaded, there is an event-loop spin before the
> consumer of the promise has the .then() handler called.  This means it is
> typically *after* DOMContentLoaded has completed and scripts have started
> executing.

Thinking more about this, I think we should do the best we can to *not* promise we will deliver callbacks synchronously and at a specific time.  In a future world we might want chats to live in their own process, for example.  I think it is fine for our "contract" to be that the promise will resolve/callback will be made at some point after the window has been shown an started loading, and that's it.

Comment 6

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

Attachment: 0001-Bug-1002914-refactor-the-chat-window-so-it-can-be-us.patch

This is a more ambitious patch, but I think it is closer to the "correct" way of implementing it.

* There is a new Chat.jsm which everyone uses instead of hitting the chatbar directly; after chatting with Shane we felt it was best to decouple the chat API from the window - thus the chatbar becomes an implementation detail internal to Chat.jsm.

* I dropped .promiseChatCreated() and reinstated the callbacks - it is important that the callback happen immediately and not after event-loop spins, else Loop will not be able to be sure it is injecting its API before content has loaded and Social might install the error listener after an error page loads.  The callbacks now happen earlier - directly in the constructor instead of in DOMContentLoaded.  There is still .promiseChatLoaded() as a convenience.

* Social:FocusChat has been renamed to Chat:Focus and is never hidden nor disabled - the oncommand handler just does nothing when no chats are available.  Currently this is only used by a <key> binding, so this should not cause any issues.

* SocialChatBar has almost been totally removed - it could be totally removed if tests are refactored - which is probably worthwhile but not in this patch - it's big enough as it is - probably a "part 2"

* I've flagged that the .removeAll() method on the chatbar should be removed, but as above, this will require test refactoring that doesn't seem worthwhile in this initial patch - probably a "part 2"

* Ideally some of the existing social chat tests should be split into chat specific tests - probably a "part 2"

* I've still done nothing with socialChatActivity - I think we can fix that in a followup.

Tests pass locally for me but try appears down.  I think this is getting close to ready, but sticking with feedback requests until Gavin gives one ;)

Comment 7

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

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

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8415782 [details] [diff] [review]
0001-Bug-1002914-refactor-the-chat-window-so-it-can-be-us.patch

I haven't looked at the patch (too big to be able to reasonably do this week!), but all of your descriptions of the changes sound great. If you need me to dig into the code changes (or have any in particular you want me to look at), let me know.

Comment 9

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8415782 [details] [diff] [review]
0001-Bug-1002914-refactor-the-chat-window-so-it-can-be-us.patch

overall looks good.  I still think we can just drop socialChatActivity.

Comment 10

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

Attachment: 0001-Bug-1002914-part-1-refactor-the-chat-window-so-it-ca.patch

Basically the same as the previous patch, but I took your advice and removed support for the activity event.

Comment 11

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

Attachment: 0002-Bug-1002914-part-2-remove-SocialChatBar-add-chat-spe.patch

The "part 2":

* Removes SocialChatBar entirely, and refactors the social tests to account for that.
* Adds chat-specific tests - basically "moved" them from social into chat (although done with Tasks, so they are *much* easier to read.)

Try with both these patches: https://tbpl.mozilla.org/?tree=Try&rev=65b3f3d4a906

Comment 12

[Maire Reavy [:mreavy]]

Thanks for doing this, Mark!

Comment 13

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8416355 [details] [diff] [review]
0001-Bug-1002914-part-1-refactor-the-chat-window-so-it-ca.patch

r+ with a fix for Chat:Focus import (or we can discuss on irc later)

The Chat:Focus is going to import Chat.jsm immediately, feels like there should be a better way to handle that so it loads lazily.  


other considerations (non-blocking)

Should we change the window name from Social:Chat to something else?

while we're at it, I think we should consider a "detached" mode for Chat.open.

Comment 14

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8416356 [details] [diff] [review]
0002-Bug-1002914-part-2-remove-SocialChatBar-add-chat-spe.patch

lgtm.  I couldn't access the try run though.

Comment 15

[Shane Caraveo (:mixedpuppy)]

ah, try is back, looks like there is a failure that needs to be fixed.

Comment 16

[Shane Caraveo (:mixedpuppy)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #13)
> Comment on attachment 8416355 [details] [diff] [review]
> 0001-Bug-1002914-part-1-refactor-the-chat-window-so-it-ca.patch
> 
> r+ with a fix for Chat:Focus import (or we can discuss on irc later)
> 
> The Chat:Focus is going to import Chat.jsm immediately, feels like there
> should be a better way to handle that so it loads lazily.  

nix that comment, chat.jsm shouldn't get loaded until oncommand is fired.

Comment 17

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

FTR, this patch broke devtools tests due to a combination of:

* devtools and the chat window both attempt to use ctrl+shift+c - devtools to open the console.
* this patch makes the Chat:Focus command unhidden by default, meaning the keys now conflict immediately as the browser starts.  Previously the keys only conflicted once a chat window was opened.

The fact the keys already conflicted is a problem even though it didn't make the tests fail - it means that "suddenly" the user finds a key combination that worked before no longer works - so IMO the correct fix is *not* to simply default the chat key to hidden by default - so I changed the shortcut to be ctrl+alt+c - which I'll push with the initial patches unless I hear objections in the meantime.

Try with this change: https://tbpl.mozilla.org/?tree=Try&rev=2fda93530051

Comment 18

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

(In reply to Mark Hammond [:markh] from comment #17)
...
> simply default the chat key to hidden by default - so I changed the shortcut
> to be ctrl+alt+c - which I'll push with the initial patches unless I hear
> objections in the meantime.

Sadly devtools uses shift+ctrl+c everywhere except OSX, where it uses ctrl+alt+c!  So I just reversed both of them - Chat:Focus using ctrl+shift+c on OSX, ctrl+alt+c everywhere else.  Green try at https://tbpl.mozilla.org/?tree=Try&rev=1dfc8ca5468c

Comment 19

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

Thanks!

(In reply to Shane Caraveo (:mixedpuppy) from comment #13)
> other considerations (non-blocking)
> 
> Should we change the window name from Social:Chat to something else?

If I could think of a better name, I would have.  I could only come up with Chat:Window or Chat:DetachedWindow, neither of which sound good - so we can do this later.

> while we're at it, I think we should consider a "detached" mode for
> Chat.open.

Given we don't have a real use-case yet, I reckon this should also be done in a followup once we have the need.

https://hg.mozilla.org/integration/fx-team/rev/7824a3826963
https://hg.mozilla.org/integration/fx-team/rev/2f3f35b8cea3

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7824a3826963
https://hg.mozilla.org/mozilla-central/rev/2f3f35b8cea3

Comment 21

[Ryan VanderMeulen [:RyanVM]]

I had to back this out because it wasn't playing well with Dao's landing in bug 805068.
https://hg.mozilla.org/integration/fx-team/rev/8be0e21fd300

https://tbpl.mozilla.org/php/getParsedLog.php?id=39208680&tree=Fx-Team
https://tbpl.mozilla.org/php/getParsedLog.php?id=39227611&tree=Fx-Team

Comment 22

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

Re-pushed with test tweaks to prevent intermittent oranges - heavily retriggered try at https://tbpl.mozilla.org/?tree=Try&rev=4f69fc42dcf7

https://hg.mozilla.org/integration/fx-team/rev/382866d05362
https://hg.mozilla.org/integration/fx-team/rev/39552f7a67e2

Comment 23

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/382866d05362
https://hg.mozilla.org/mozilla-central/rev/39552f7a67e2

Comment 24

[u279076]

Verified fixed in Firefox 33 and 34.