navigator.mozSocial.openChatWindow's callback should be called only once

RESOLVED FIXED in Firefox 19

Status

()

Firefox
SocialAPI
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: florian, Assigned: markh)

Tracking

Trunk
Firefox 19
Points:
---

Firefox Tracking Flags

(firefox18-)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

5 years ago
It's currently called each time a DOMContentLoaded event fires in the chat window. That surprised me, and I think (gavin agreed on IRC) it's not the expected behavior.
I would expect the mozSocial code to forget the callback after it's been called once.
(Assignee)

Comment 1

5 years ago
Created attachment 677665 [details] [diff] [review]
Remove DOMContentLoaded handler and nuke callback for good measure.
Assignee: nobody → mhammond
Attachment #677665 - Flags: review?(mixedpuppy)
(Assignee)

Updated

5 years ago
tracking-firefox18: --- → ?

Updated

5 years ago
tracking-firefox18: ? → +
Comment on attachment 677665 [details] [diff] [review]
Remove DOMContentLoaded handler and nuke callback for good measure.

Only question I have around this change is whether we should consider a situation where two different content area's open the same chat (e.g. sidebar and worker), and need a window ref in return.  I believe the API docs would lead developers to believe they would get at least one callback in each case.  If the chat is already open, should openChatWindow return the window ref or perhaps call the callback with the ref?
Attachment #677665 - Flags: review?(mixedpuppy) → review+
(Assignee)

Comment 3

5 years ago
(In reply to Shane Caraveo (:mixedpuppy) from comment #2)
> Only question I have around this change is whether we should consider a
> situation where two different content area's open the same chat (e.g.
> sidebar and worker), and need a window ref in return.  I believe the API
> docs would lead developers to believe they would get at least one callback
> in each case.  If the chat is already open, should openChatWindow return the
> window ref or perhaps call the callback with the ref?

If the chat is already open, openChat arranges to still make a callback (this is an old bug we already fixed and has tests.

But you are correct in that there is a problem if the 2 chats are requested *before* the load event has fired - in that case the second one would probably be called too early.  But that behaviour wasn't introduced by this patch, so how about we open a new bug for this (it would involve turning self._callback into an array of callbacks, and have openChat check the loadState and either callback immediately or append the new callback to the array.
(Assignee)

Comment 4

5 years ago
Created attachment 677969 [details] [diff] [review]
updated so new chat window requests before onload have fired are called back onload

I think this one handles all those cases correctly - there is no test for the "new chat requested before onload has fired" as I can't see how to test that reliably - but there are tests that (a) callback isn't fired twice and (b) second request for a chat does callback.
Attachment #677665 - Attachment is obsolete: true
Attachment #677969 - Flags: review?(mixedpuppy)
(Assignee)

Comment 5

5 years ago
Comment on attachment 677969 [details] [diff] [review]
updated so new chat window requests before onload have fired are called back onload

Hoping jaws will clear his review queue quicker!
Attachment #677969 - Flags: review?(mixedpuppy) → review?(jaws)
Comment on attachment 677969 [details] [diff] [review]
updated so new chat window requests before onload have fired are called back onload

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

::: browser/base/content/socialchat.xml
@@ +56,5 @@
>          <parameter name="aCallback"/>
>          <body><![CDATA[
> +          // callbacks to be called when onload fires - more might be added
> +          // if the same chat is requested before onload has fired.  Set back
> +          // to null in, so null means onload has already fired and new

// to null in openChat, so null means onload has already fired and new

@@ +328,5 @@
>                  this.selectedChat = cb;
>                if (cb.collapsed)
>                  this.showChat(cb);
> +              if (aCallback) {
> +                if (cb._callbacks == null) {

if (!cb._callbacks) {
Attachment #677969 - Flags: review?(jaws) → review+
Actually, you can keep the explicit check for null here.
(Assignee)

Comment 8

5 years ago
Created attachment 681299 [details] [diff] [review]
rebased and new test

Sorry - the old patch was out of date and needed reworking to make work - enough that I can't carry the r+ forward.

There is now an initChatBox method instead of the constructor, and the timing of adding the unload event was changed (it is now added in DOMContentLoaded).  Also, I added a test that the sub-iframe in the chatboxes firing an unload event didn't cause our event handlers to also be removed
Attachment #677969 - Attachment is obsolete: true
Attachment #681299 - Flags: review?(jaws)
Comment on attachment 681299 [details] [diff] [review]
rebased and new test

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

::: browser/base/content/socialchat.xml
@@ +347,5 @@
> +          let iframeWindow = aChatBox.iframe.contentWindow;
> +          aChatBox.addEventListener("DOMContentLoaded", function DOMContentLoaded() {
> +            aChatBox.removeEventListener("DOMContentLoaded", DOMContentLoaded);
> +            aChatBox.isActive = !aChatBox.minimized;
> +            for (let cb of aChatBox._callbacks) {

s/cb/callback/ since cb is also used as a shortname for chatbox.

@@ +384,5 @@
> +                if (cb._callbacks == null) {
> +                  // DOMContentLoaded has already fired, so callback now.
> +                  aCallback(cb.iframe.contentWindow);
> +                } else {
> +                  // onload for this chat is yet to fire...

// DOMContentLoaded for this chat has yet to fire.

::: browser/base/content/test/browser_social_chatwindow.js
@@ +363,5 @@
>        // The nub should lose the activity flag here too
>        todo(!chatbar.nub.hasAttribute("activity"), "Bug 806266 - nub should no longer have activity");
>        // TODO: tests for bug 806266 should arrange to have 2 chats collapsed
>        // then open them checking the nub is updated correctly.
> +      // Now we wlll go and change the embedded iframe in the second chat and

s/wlll/will/

@@ +375,5 @@
> +        executeSoon(function() {
> +          let evt = iframe2.contentDocument.createEvent("CustomEvent");
> +          evt.initCustomEvent("socialChatActivity", true, true, {});
> +          iframe2.contentDocument.documentElement.dispatchEvent(evt);
> +          ok(second.hasAttribute("activity"), "second chat still have activity after unloading sub-iframe");

s/have/has/
Attachment #681299 - Flags: review?(jaws) → review+
(Assignee)

Comment 11

5 years ago
Created attachment 681837 [details] [diff] [review]
As landed
Attachment #681299 - Attachment is obsolete: true
Attachment #681837 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/8072a58a9e86
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 19
Is this Social API fix still desired on FF18? If so, please nominate asap.
(Assignee)

Comment 14

5 years ago
personally, I think we can live without this on 18 as it is fairly easy for providers to work around.  Gavin, what do you think?
Yeah, I think this hasn't been much of a problem in practice (Facebook doesn't use subframes in their chat panels, and doesn't reload them, IIRC).
tracking-firefox18: + → -
You need to log in before you can comment on or make changes to this bug.