[de-xbl] convert chat tooltip (imtooltip.xml) to a custom element <tooltip is="chat-tooltip"/>
Categories
(Thunderbird :: Instant Messaging, task)
Tracking
(Not tracked)
People
(Reporter: mkmelin, Assigned: mkmelin)
References
Details
Attachments
(2 files, 9 obsolete files)
(deleted),
patch
|
mkmelin
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review |
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.
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.
Assignee | ||
Comment 2•6 years ago
|
||
I think this is related to bug 492326 comment 21.
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.
Assignee | ||
Updated•6 years ago
|
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.
Comment 7•6 years ago
|
||
Comment on attachment 9031418 [details] [diff] [review]
bug1506529_chat-tooltip.patch
Florian offered to handle this for me, he knows this code much better.
Assignee | ||
Comment 8•6 years ago
|
||
Rebased on top of 1519091 (and other landings)
Assignee | ||
Comment 9•6 years ago
|
||
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•6 years ago
|
||
(In reply to Magnus Melin [:mkmelin] from comment #9)
Created attachment 9044157 [details] [diff] [review]
bug1506529_chat-tooltip.patchRefreshed. 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.
Assignee | ||
Comment 11•6 years ago
|
||
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•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).
Updated•5 years ago
|
Assignee | ||
Comment 13•5 years ago
|
||
(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.
Assignee | ||
Comment 14•5 years ago
|
||
Let's get this fixed.
Assignee | ||
Comment 15•5 years ago
|
||
(Forgot to run lint)
Comment 16•5 years ago
|
||
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.
Assignee | ||
Comment 17•5 years ago
|
||
No, this bug hasn't landed yet so it can't possibly have caused any problems.
Comment 18•5 years ago
|
||
(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•5 years ago
|
||
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.
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 22•5 years ago
|
||
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.
Assignee | ||
Updated•5 years ago
|
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.
Assignee | ||
Comment 24•5 years ago
|
||
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 27•5 years ago
|
||
Usually we do a rename. Which one do you prefer?
Updated•5 years ago
|
Comment 29•5 years ago
|
||
Pushed by mozilla@jorgk.com:
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
Updated•5 years ago
|
Description
•