1538549 - [de-xbl] convert contact binding to <richlistitem is="chat-contact">

Status: RESOLVED FIXED

(Thunderbird :: Instant Messaging, task)

Description

[Magnus Melin [:mkmelin]]

Convert the imcontact binding to customized built-in.

https://searchfox.org/comm-central/rev/47187196fbf02cc0d635371b3879ddf5a49f491c/mail/components/im/content/imcontact.xml#17

https://searchfox.org/comm-central/rev/47187196fbf02cc0d635371b3879ddf5a49f491c/mail/components/im/content/chat.css#14

https://searchfox.org/comm-central/rev/47187196fbf02cc0d635371b3879ddf5a49f491c/mail/components/im/content/imgroup.xml#58

Comment 1

[Khushil Mistry [:khushil324]]

Attachment: de-xbl_imcontact.patch

Comment 2

[Magnus Melin [:mkmelin]]

Comment on attachment 9056230 [details] [diff] [review]
de-xbl_imcontact.patch

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

Should hg mv --after mail/components/im/content/imcontact.xml mail/components/im/content/chat-contact.js 

This looks good overall I think

::: mail/components/im/content/chat-contact.js
@@ +12,5 @@
> +  /**
> +   * The MozChatContact widget displays contact information about user under
> +   * chat-groups, online contacts and offline contacts: i.e. icon and username.
> +   * On double clicking the element, it gets removed from chat-groups and chat-conv
> +   * element gets added under chat-group named conversation.

... it gets moved into the conversations

@@ +45,5 @@
> +      });
> +
> +      this.addEventListener("mousedown", (event) => {
> +        if (!this.hasAttribute("aliasing") && this.canOpenConversation() &&
> +          event.originalTarget.classList.contains("startChatBubble")) {

for these things we indent

if (!this.hasAttribute("aliasing") && this.canOpenConversation() &&
    event.originalTarget.classList.contains("startChatBubble")) {

@@ +54,5 @@
> +
> +      this.addEventListener("click", (event) => {
> +        if (!this.hasAttribute("aliasing") && this.canOpenConversation() &&
> +          event.detail == 2 &&
> +          event.originalTarget.getAttribute("anonid") != "expander") {

is this dead code? I don't see an anonid="expander" in the tree anywhere

::: mail/components/im/content/chat-messenger.js
@@ +19,5 @@
>    this.menu = aXulMenu;
>    let localName = this.target.localName;
> +  document.getElementById("contactlistbox").selectedItem = this.target;
> +  this.onContact = localName == "richlistitem" &&
> +    this.target.getAttribute("is") == "chat-contact";

would add some parenthesis here for readability

Comment 3

[Magnus Melin [:mkmelin]]

Comment on attachment 9056230 [details] [diff] [review]
de-xbl_imcontact.patch

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

::: mail/components/im/content/chat-contact.js
@@ +159,5 @@
> +      }).bind(this);
> +      this.parentNode.addEventListener("select", this._parentSelectListener);
> +    }
> +
> +    finishAliasing(aSave) {

and please make this save instead of aSave

Comment 4

[Khushil Mistry [:khushil324]]

Attachment: Bug-1538549_de-xbl_imcontact.patch

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=b6037fac327ac9cd3754776495154a520972d2c7

Comment 5

[Khushil Mistry [:khushil324]]

Attachment: Bug-1538549_de-xbl_imcontact.patch

Comment 6

[Khushil Mistry [:khushil324]]

Patch updated with ESLint correction. Magnus, can you confirm the patch after seeing the test results on try-comm?

Comment 7

[Magnus Melin [:mkmelin]]

NOt the best of try runs but looks ok to me (and the linting failure is now fixed)

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/d760edcda279
[de-xbl] convert contact binding to custom element. r=mkmelin DONTBUILD

Comment 9

[Magnus Melin [:mkmelin]]

This never made it into esr 68. I'm not sure why.

Comment 10

[Khushil Mistry [:khushil324]]

There were other related xbl elements: chat-group and chat-conversation. We need to merge all three or none of them as they are co-related. And some other functionalities were dependent upon that. So we didn't merge these patches.

Comment 11

[Magnus Melin [:mkmelin]]

I see now. I was looking at the wrong time window. It all makes sense.

Comment 12

[Jorg K (CEST = GMT+2)]

Also keep in mind that chat in TB 68 is working based on a series of backouts, see bug 1555136 comment #21. Please don't touch chat in TB 68 or it will all fall apart.