Magnus Melin [:mkmelin] Assignee
	Description • 6 years ago

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.

	Magnus Melin [:mkmelin] Assignee
	Comment 1 • 6 years ago

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

	Magnus Melin [:mkmelin] Assignee
	Comment 2 • 6 years ago

I think this is related to bug 492326 comment 21.

	Magnus Melin [:mkmelin] Assignee
	Comment 3 • 6 years ago

Missing a 1. bug 1492326 comment 21.

	Magnus Melin [:mkmelin] Assignee
	Comment 4 • 6 years ago

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.

	Magnus Melin [:mkmelin] Assignee
	Comment 5 • 6 years ago

Dropped the /gre

	Magnus Melin [:mkmelin] Assignee
	Comment 6 • 6 years ago

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.

	Florian Quèze [:florian]
	Comment 12 • 6 years ago

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).

	Alessandro Castellani [:aleca]
	Comment 23 • 5 years ago

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.