Closed Bug 1002914 Opened 8 years ago Closed 8 years ago

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

Categories

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

defect

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: markh, Assigned: markh)

References

Details

(Whiteboard: [s=mlpnightly1, p=.25][qa+])

Attachments

(2 files, 2 obsolete files)

We want the loop client to be able to use the chat window without going via the social API.
Attached patch refactor-social.patch (obsolete) — Splinter Review
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.
Attachment #8414222 - Flags: feedback?(mixedpuppy)
Attachment #8414222 - Flags: feedback?(gavin.sharp)
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?
Attachment #8414222 - Flags: feedback?(mixedpuppy) → feedback+
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?
Flags: needinfo?(mixedpuppy)
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.
(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.
Flags: needinfo?(mixedpuppy)
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 ;)
Attachment #8414222 - Attachment is obsolete: true
Attachment #8414222 - Flags: feedback?(gavin.sharp)
Attachment #8415782 - Flags: feedback?(mixedpuppy)
Attachment #8415782 - Flags: feedback?(gavin.sharp)
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.
Attachment #8415782 - Flags: feedback?(gavin.sharp) → feedback+
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.
Attachment #8415782 - Flags: feedback?(mixedpuppy) → feedback+
Basically the same as the previous patch, but I took your advice and removed support for the activity event.
Attachment #8415782 - Attachment is obsolete: true
Attachment #8416355 - Flags: review?(mixedpuppy)
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
Attachment #8416356 - Flags: review?(mixedpuppy)
Thanks for doing this, Mark!
Assignee: nobody → mhammond
Priority: -- → P1
Target Milestone: --- → mozilla32
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.
Attachment #8416355 - Flags: review?(mixedpuppy) → review+
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.
Attachment #8416356 - Flags: review?(mixedpuppy) → review+
ah, try is back, looks like there is a failure that needs to be fixed.
(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.
Whiteboard: [s=mlpnightly1, p=.25]
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
(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
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
Status: NEW → ASSIGNED
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla32 → ---
Blocks: 1000007
No longer blocks: 1000012
https://hg.mozilla.org/mozilla-central/rev/382866d05362
https://hg.mozilla.org/mozilla-central/rev/39552f7a67e2
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Verified fixed in Firefox 33 and 34.
Status: RESOLVED → VERIFIED
QA Contact: anthony.s.hughes
Whiteboard: [s=mlpnightly1, p=.25] → [s=mlpnightly1, p=.25][qa+]
You need to log in before you can comment on or make changes to this bug.