1100284 - Loop rooms shouldn't allow opening the same room more than once

Status: VERIFIED FIXED

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

Description

[Mark Banner (:standard8)]

Currently, if I select a room twice in the panel, then the room opens and connects to itself.

Really, we should focus the existing window.

Comment 1

[Marco Mucci [:MarcoM]]

Added to IT 36.3

Comment 2

[Mike de Boer [:mikedeboer]]

Attachment: Patch 1: use a more canonical window ID for chat windows

Well, this seems to get us there, except for detached windows... but that's a bug in Social code.

Comment 3

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2: also support detached chat windows in duplicate chat window handling

Comment 4

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8524739 [details] [diff] [review]
Patch 1: use a more canonical window ID for chat windows

On the outset it looks fine, but I'm not really familiar with this code, probably better to get someone else to review.  My initial question is whether gLastWindowId is still necessary at all.

Comment 5

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2.1: also support detached chat windows in duplicate chat window handling

Fixed a whoopsie... Carrying over r=mixedpuppy.

Thanks much, Shane!

Comment 6

[Mike de Boer [:mikedeboer]]

Attachment: Patch 1.1: use a more canonical window ID for chat windows

Comment 7

[Matthew N. [:MattN]]

Comment on attachment 8525975 [details] [diff] [review]
Patch 1.1: use a more canonical window ID for chat windows

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

This seems reasonable. rs=me

::: browser/components/loop/MozLoopService.jsm
@@ +761,5 @@
>    openChatWindow: function(conversationWindowData) {
>      // So I guess the origin is the loop server!?
>      let origin = this.loopServerUri;
> +    // Try getting a window ID that can (re-)identify this conversation, or resort
> +    // to a globally unique one as a lasy resort.

Typo: s/lasy/last/

@@ +765,5 @@
> +    // to a globally unique one as a lasy resort.
> +    // XXX We can clean this up once rooms and direct contact calling are the only
> +    //     two modes left.
> +    let windowId = ("contact" in conversationWindowData) ?
> +                   conversationWindowData.contact._guid :

Perhaps the guid should become a public property (i.e. drop the _ prefix) in the future?

Comment 8

[Mike de Boer [:mikedeboer]]

(In reply to Matthew N. [:MattN] (UTC+1 until Nov. 22; away Nov. 14-19) from comment #7)
> Typo: s/lasy/last/

Fixed.

> Perhaps the guid should become a public property (i.e. drop the _ prefix) in
> the future?

Myeah, that's prolly a good idea. But that would require a DB migration, though. :/

Pushed to fx-team as:

https://hg.mozilla.org/integration/fx-team/rev/a7621853f055
https://hg.mozilla.org/integration/fx-team/rev/a46accc80752

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out for xpcshell bustage:
remote:   https://hg.mozilla.org/integration/fx-team/rev/7228e1d598ee
remote:   https://hg.mozilla.org/integration/fx-team/rev/6fec67fdc58b

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=1257652&repo=fx-team

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/fx-team/rev/e8f45cf7b043
https://hg.mozilla.org/integration/fx-team/rev/c773c6be9270

Updated to resolve xpcshell failure (_guid was undefined, so fallback to last window id) r=abr on IRC
Tested locally - lets you in twice without the patch; ignores second attempt to join with this patch.

Mike: just double-check when you get a chance.
(I added "|| gLastWindowId++" after "....contacts._guid")

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

And back out again... https://hg.mozilla.org/integration/fx-team/rev/1a9ce687110d


https://treeherder.mozilla.org/ui/logviewer.html#?job_id=1262064&repo=fx-team

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

fixed second xpcshell bustage:

https://hg.mozilla.org/integration/fx-team/rev/33a80305f1b4
https://hg.mozilla.org/integration/fx-team/rev/f5c0481fe56b

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

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/33a80305f1b4
https://hg.mozilla.org/mozilla-central/rev/f5c0481fe56b

Comment 14

[u279076]

(In reply to Carsten Book [:Tomcat] from comment #13)
> https://hg.mozilla.org/mozilla-central/rev/33a80305f1b4
> https://hg.mozilla.org/mozilla-central/rev/f5c0481fe56b

Is test coverage sufficient or will this need further test coverage?

Comment 15

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8525970 [details] [diff] [review]
Patch 2.1: also support detached chat windows in duplicate chat window handling

Approval Request Comment
[Risks and why]: Loop rooms code needed for 35 pref-on
Connecting a call to yourself is both confusing and useless (and causes other potential problems).
[String/UUID change made/needed]: none

Comment 16

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6d5163d6bf13

Comment 17

[Vasilica Tamas, QA [:vtamas]]

I was able to reproduce this issue on Firefox 36.a1 (2014-11-23) using Windows 7 x64.

Verified fixed on latest Firefox 36.0a2 (2014-12-17) and Firefox 35 Beta 4 (20141216120925) using Windows 7 x64 and Ubuntu 14.04 x86.