1506529 - [de-xbl] convert chat tooltip (imtooltip.xml) to a custom element <tooltip is="chat-tooltip"/>

Status: RESOLVED FIXED

(Thunderbird :: Instant Messaging, task)

Description

[Magnus Melin [:mkmelin] (away, back Aug 4)]

Attachment: bugXXX_chat-tooltip.patch

Convert the chat tooltip (imtooltip.xml) to custom element.

Not quite ready yet, I keep getting 

JavaScript error: chrome://chat/content/chat-tooltip.js, line 267: NS_ERROR_XPC_BAD_CONVERT_JS: Could not convert JavaScript argument arg 0 [nsIObserverService.addObserver]

I thought MozXULElement.implementCustomInterface(MozChatTooltip, [Ci.nsIObserver]); would handle making this and nsIObserver, and it does seem to add the QI, just that it still doesn't work for some reason.

Comment 1

[Magnus Melin [:mkmelin] (away, back Aug 4)]

Anything obvious I'm missing here that's causing the NS_ERROR_XPC_BAD_CONVERT_JS?
The class does have an observe method.

Comment 2

[Magnus Melin [:mkmelin] (away, back Aug 4)]

I think this is related to bug 492326 comment 21.

Comment 3

[Magnus Melin [:mkmelin] (away, back Aug 4)]

Missing a 1. bug 1492326 comment 21.

Comment 4

[Magnus Melin [:mkmelin] (away, back Aug 4)]

Attachment: bug1506529_chat-tooltip.patch

Going for the helper object as observer approach. 
This seems to be working. 

I don't understand what's up with not being able to import imStatusUtils.jsm... so I just added the Status as a eslint global.

Comment 5

[Magnus Melin [:mkmelin] (away, back Aug 4)]

Attachment: bug1506529_chat-tooltip.patch

Dropped the /gre

Comment 6

[Magnus Melin [:mkmelin] (away, back Aug 4)]

Ping on the review. This is difficult to review of course. I'd mostly check it out and see if it seems to work. Then sanity check the code.

Comment 7

[Patrick Cloke [:clokep]]

Comment on attachment 9031418 [details] [diff] [review]
bug1506529_chat-tooltip.patch

Florian offered to handle this for me, he knows this code much better.

Comment 8

[Magnus Melin [:mkmelin] (away, back Aug 4)]

Attachment: bug1506529_chat-tooltip.patch

Rebased on top of 1519091 (and other landings)

Comment 9

[Magnus Melin [:mkmelin] (away, back Aug 4)]

Attachment: bug1506529_chat-tooltip.patch

Refreshed. Please review within a reasonable time. It's difficult to get a reasonable overview of what to tackle next when too many patches are in the review pipeline, in combination with general bustages.

Comment 10

[Florian Quèze [:florian]]

(In reply to Magnus Melin [:mkmelin] from comment #9)

Created attachment 9044157 [details] [diff] [review]
bug1506529_chat-tooltip.patch

Refreshed. Please review within a reasonable time.

Unless you want a review by code-inspection only, it's going to be hard (if not impossible) to review before bug 1528907 is fixed.

Comment 11

[Magnus Melin [:mkmelin] (away, back Aug 4)]

Attachment: bug1506529_chat-tooltip.patch

Unbitrotted again.
Can we get this reviewed and landed? It's actually the last of the chat xbl bindings, yay!

My previous comment still applies: yes not super easy to review, but test it out and if there aren't any obvious problems that come to mind, it's probably just easier to do follow-ups if needed.

Comment 12

[Florian Quèze [:florian]]

Comment on attachment 9053052 [details] [diff] [review]
bug1506529_chat-tooltip.patch

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

It seems you lost the common/content/customElements.js change with the latest rebase.

I couldn't test locally despite applying the patch (it hasn't bitrotted yet! :-)), because the account wizard is busted again, with the following errors:
JavaScript error: chrome://messenger/content/chat/imAccountWizard.js, line 13: TypeError: document.getElementById(...) is null
JavaScript error: chrome://messenger/content/chat/imAccountWizard.xul, line 1: TypeError: accountWizard is undefined

::: chat/content/chat-tooltip.js
@@ +30,5 @@
> +             topic == "account-buddy-status-detail-changed" ||
> +             topic == "account-buddy-display-name-changed" ||
> +             topic == "account-buddy-icon-changed")) {
> +          this.updateTooltipFromBuddy(this.buddy);
> +        } else if (topic == "contact-preferred-buddy-changed" &&

This notification won't happen, and this.contact will never be set in Thunderbird, you can just drop this case.

@@ +47,5 @@
> +  attributeChangedCallback() {
> +    this._updateAttributes();
> +  }
> +
> +  _updateAttributes() {

The _updateAttributes method is called only once, so it should just be inlined inside attributeChangedCallback.

@@ +56,4 @@
>  
> +    let left = this.getAttribute("left");
> +    if (left) {
> +      this.querySelectorAll(".statusTypeIcon").setAttribute("left", left);

querySelector*All*.setAttribute looks like a typo.

@@ +60,5 @@
> +    } else {
> +      this.querySelector(".statusTypeIcon").removeAttribute("left");
> +    }
> +
> +    let displayname = this.getAttribute("displayname");

There's too much duplication here. How about something like this?

let attributes = {
  status: "status",
  left: "statutsTypeIcon",
  displayname: "tooltipDisplayName",
  iconPrpl: "tooltipProtoIcon",
  noTopic: "tooltipMessage",
};

for (let attr in attributes) {
  let val = this.getAttribute(attr);
  let elements = this.getElementsByClassName(attributes[attr]);
  for (e of elements) {
    if (val) {
      e.setAttribute(attr, val);
    } else {
      e.removeAttribute(attr);
    }
  }
}

@@ -18,5 @@
> -      <xul:vbox id="largeTooltip">
> -        <xul:hbox align="start" crop="end" flex="1">
> -          <xul:vbox flex="1">
> -            <xul:stack>
> -              <!-- The box around the user icon is a workaround for bug 955673. -->

I think we should preserve this comment.

@@ +89,5 @@
> +        <hbox align="start" crop="end" flex="1">
> +          <vbox flex="1">
> +            <stack>
> +              <box class="userIconHolder">
> +                <image class="userIcon"></image>

Is there a requirement to write <foo></foo> rather than <foo/>? This makes the markup look heavier than it actually is.

@@ +113,5 @@
> +            <column flex="1"></column>
> +          </columns>
> +          <rows class="tooltipRows"></rows>
> +        </grid>
> +        <vbox class="tooltipBuddies"></vbox>

You can remove this tooltipBuddies vbox.

@@ +204,4 @@
>  
> +  _onPopupHiding() {
> +    this.buddy = null;
> +    this.contact = null;

You can remove this line.

@@ +214,5 @@
>  
> +    let buddies = this.querySelector(".tooltipBuddies");
> +    while (buddies.hasChildNodes()) {
> +      buddies.lastChild.remove();
> +    }

And these 4 lines.

@@ +257,5 @@
> +  get buddy() {
> +    return this._buddy;
> +  }
> +
> +  set contact(val) {

The contact setter/getter are unused in Thunderbird.

@@ +405,2 @@
>  
> +  updateTooltipFromContact(aContact) {

updateTooltipFromContact is dead code in Thunderbird, as contact support was never ported from Instantbird (it was kinda waiting on the TB address book rewrite that never arrived).

Comment 13

[Magnus Melin [:mkmelin] (away, back Aug 4)]

(In reply to Florian Quèze [:florian] from comment #12)

Is there a requirement to write <foo></foo> rather than <foo/>? This makes
the markup look heavier than it actually is.

No. And yes. The converter doesn't use the self-closing form. I think because custom elements are not allowed to be self-closing, so say in the future foo is converted to a custom element, that would be invalid if written as self-closing. I guess it currently works with the short form, but it might break in the future.

Comment 14

[Magnus Melin [:mkmelin] (away, back Aug 4)]

Attachment: bug1506529_chat-tooltip.patch

Let's get this fixed.

Comment 15

[Magnus Melin [:mkmelin] (away, back Aug 4)]

Attachment: bug1506529_chat-tooltip.patch

(Forgot to run lint)

Comment 16

[WaltS48 [:walts48]]

Is this why I don't see a participant tooltip when hovering the nick in a conversation in TB 69.0b1?

I see bug 956579 was fixed for TB 29.0.

It is working in TB 60.8.0.

Comment 17

[Magnus Melin [:mkmelin] (away, back Aug 4)]

No, this bug hasn't landed yet so it can't possibly have caused any problems.

Comment 18

[WaltS48 [:walts48]]

(In reply to Magnus Melin [:mkmelin] from comment #17)

No, this bug hasn't landed yet so it can't possibly have caused any problems.

Sorry, I wasn't clear about my query.

Is the reason I don't see a tooltip because this hasn't landed yet, and the chat tooltip code still has xbl?

Comment 19

[WaltS48 [:walts48]]

The tooltip doesn't appear in 69.0a1 either, so that is probably why I don't see it in 69.0b1 or 70.0a1.

Comment 20

[Magnus Melin [:mkmelin] (away, back Aug 4)]

I don't think so.

Comment 21

[Magnus Melin [:mkmelin] (away, back Aug 4)]

Florian: ping on the review. I'd like to get this landed.

Comment 22

[Magnus Melin [:mkmelin] (away, back Aug 4)]

Attachment: bug1506529_chat-tooltip.patch

Let's get this landed soon. Alex, can you do the review?
It's a bit of a special custom element... Can't (apparently) do the normal customizable built-in definition to inherit from "tooltip" - but it works as one anyway. Can't also check children for it since the built in will confuse us, and the event handlers must be set up in the constructor for it to work.

Comment 23

[Alessandro Castellani [:aleca]]

Comment on attachment 9086966 [details] [diff] [review]
bug1506529_chat-tooltip.patch

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

It looks good and it works as expected.
Just a few nit style fixes if you want to address them.

r=aleca

::: chat/content/chat-tooltip.js
@@ +52,5 @@
> +      };
> +
> +      this.addEventListener("popupshowing", (event) => {
> +        if (!this._onPopupShowing()) {
> +         event.preventDefault();

Fix indentation

@@ +90,5 @@
> +      if (localName == "richlistitem" && elt.getAttribute("is") == "chat-group") {
> +        return false;
> +      }
> +
> +      if (localName == "richlistitem" && elt.getAttribute("is") == "chat-imconv" && elt.conv) {

Do you want to go on another line here, maybe?
It doesn't go over 100 ch, but other lines are broken below 80 ch.
Up to you.

@@ +162,5 @@
> +            <vbox flex="1">
> +              <stack>
> +                <!-- The box around the user icon is a workaround for bug 955673. -->
> +                <box class="userIconHolder">
> +                  <image class="userIcon"></image>

Another nit thing, maybe we could go with self closing markup for these empty elements?

@@ +284,5 @@
> +      // so we only use it for JavaScript prpls.
> +      // This is a terrible, terrible hack to work around the fact that
> +      // ClassInfo.implementationLanguage has gone.
> +      if (!aAccount.prplAccount.wrappedJSObject)
> +        return;

Wrap in brackets.

@@ +323,5 @@
> +
> +    updateTooltipInfo(aTooltipInfo) {
> +      while (aTooltipInfo.hasMoreElements()) {
> +        let elt =
> +          aTooltipInfo.getNext().QueryInterface(Ci.prplITooltipInfo);

No need to break line here, plenty of space.

Comment 24

[Magnus Melin [:mkmelin] (away, back Aug 4)]

Attachment: bug1506529_chat-tooltip.patch

Fixed the nits except for self-closing. We want the full format not to get into a mess later (CEs can't be self-closing).

Comment 25

[Magnus Melin [:mkmelin] (away, back Aug 4)]

Land bug 1570679 first.

Comment 26

[Jorg K (CEST = GMT+2)]

That landed already.

Comment 27

[Jorg K (CEST = GMT+2)]

Attachment: bug1506529_chat-tooltip.patch - with rename of the XML to the JS file

Usually we do a rename. Which one do you prefer?

Comment 28

[Magnus Melin [:mkmelin] (away, back Aug 4)]

Let's land it with the rename.

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/2e6b9d58dad5
[de-xbl] convert chat tooltip (imtooltip.xml) binding to custom element <tooltip is="chat-tooltip"/>. r=florian,aleca