Closed Bug 1531312 Opened 6 years ago Closed 6 years ago

[de-xbl] convert account and button to custom element

Categories

(Thunderbird :: Instant Messaging, task)

Product:
Thunderbird
Component:
Instant Messaging
Type:
task
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 68.0

People

(Reporter: khushil324, Assigned: khushil324)

References

Details

Attachments

(1 file, 7 obsolete files)

De-XBL_account.patch
(deleted), patch
Details | Diff | Splinter Review
No description provided.
Blocks: tb-war-on-xbl
Component: General → Instant Messaging
Assignee: nobody → khushil324
Status: NEW → ASSIGNED
Attached patch De-XBL_account.patch (obsolete) (deleted) — — Splinter Review
Attachment #9047921 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9047921 [details] [diff] [review] De-XBL_account.patch Review of attachment 9047921 [details] [diff] [review]: ----------------------------------------------------------------- please replace all the var with let while we're here ::: chat/content/account.js @@ +7,5 @@ > +/* global MozElements, MozXULElement */ > + > +/** > + * The MozAccountRichlist widget displays the information about the > + * configured account : i.e. icon, state, name, error, checkbox for nit: no space before : @@ +204,5 @@ > + } > + } > + > + refreshConnectedLabel() { > + var bundle = document.getElementById("accountsBundle"); For this too (I think I commented how on one of your other patches), please get the bundle from javascript, not through a bundle in xul file) @@ +274,5 @@ > + this.activeButton.click(); > + } > +} > + > +customElements.define("account-richlist", MozAccountRichlist, { extends: "richlistitem" }); richlist, but extending richlistitem - doesn't sound right. Can we make this named chat-account-richlistitem (and class name MozChatAccountRichlistitem ::: mail/components/im/content/imAccounts.xul @@ +29,4 @@ > <script type="application/javascript" src="chrome://messenger/content/chat/imStatusSelector.js"/> > <script type="application/javascript" src="chrome://global/content/nsDragAndDrop.js" /> > <script type="application/javascript" src="chrome://global/content/nsTransferable.js" /> > + <script type="application/javascript" src="chrome://chat/content/account.js" /> also change the file name appropriately
Attachment #9047921 - Flags: review?(mkmelin+mozilla)

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

For this too (I think I commented how on one of your other patches), please
get the bundle from javascript, not through a bundle in xul file)

Can you elaborate on this? or is there an example of how to do it?

(In reply to Khushil Mistry [:khushil324] from comment #3)

Can you elaborate on this? or is there an example of how to do it?

Got this. It is mentioned in your review of de-xbl activity patch. Thanks.

Attached patch De-XBL_account.patch (obsolete) (deleted) — — Splinter Review
Attachment #9047921 - Attachment is obsolete: true
Attachment #9048427 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9048427 [details] [diff] [review] De-XBL_account.patch Review of attachment 9048427 [details] [diff] [review]: ----------------------------------------------------------------- Didn't try it yet ::: chat/content/chat-account-richlistitem.js @@ +92,5 @@ > + > + get autoLogin() { > + return this.hasAttribute("autologin"); > + } > + /** add a line of space in between here @@ +96,5 @@ > + /** > + * override the default accessible name > + */ > + get label() { > + return this.getAttribute('name'); double quotes please @@ +203,5 @@ > + reconnect = bundle.formatStringFromName("account.reconnectInDouble", [val1, unit1, val2, unit2], 4) > + } > + this.querySelector(".error-reconnect").textContent = reconnect; > + return reconnect; > + }).bind(this); instead of bind, you should just be able to use an array function let updateReconnect = () => { ::: mail/components/im/content/imAccounts.js @@ +44,4 @@ > let defaultID; > Services.core.init(); // ensure the imCore is initialized. > for (let acc of this.getAccounts()) { > + var elt = document.createElement("richlistitem", { is: "chat-account-richlistitem" }); let let
Attached patch De-XBL_account.patch (obsolete) (deleted) — — Splinter Review
Attachment #9048427 - Attachment is obsolete: true
Attachment #9048427 - Flags: review?(mkmelin+mozilla)
Attachment #9049270 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9049270 [details] [diff] [review] De-XBL_account.patch Review of attachment 9049270 [details] [diff] [review]: ----------------------------------------------------------------- The toolbuttons are too broken to test this atm due to bug bug 1533085, but looking at the code, there's some things yet to do. ::: chat/content/chat-account-richlistitem.js @@ +36,5 @@ > + } > + } > + // Prevent from loading an account wizzard > + event.stopPropagation(); > + }); maybe add the event listener in connectedCallback() ? @@ +40,5 @@ > + }); > + > + } > + > + connectedCallback() { add delayedConnectedCallback()? this also needs to revent adding the child more than once. if this.hasChildNodes() return @@ +196,5 @@ > + let updateReconnect = () => { > + let date = Math.round((account.timeOfNextReconnect - Date.now()) / 1000); > + let reconnect = ""; > + if (date > 0) { > + let [val1, unit1, val2, unit2] = DownloadUtils.convertTimeUnits(date); hm, DownloadUtils and Services aren't imported here. add the prevention of leaking to global scope at top of the file, then import them inside that run ./mach lint comm/chat/content/chat-account-richlistitem.js and fix the errors
Attachment #9049270 - Flags: review?(mkmelin+mozilla)
Attached patch De-XBL_account.patch (obsolete) (deleted) — — Splinter Review
Attachment #9049270 - Attachment is obsolete: true
Attachment #9049865 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9049865 [details] [diff] [review] De-XBL_account.patch Review of attachment 9049865 [details] [diff] [review]: ----------------------------------------------------------------- Bitrotted. But more importantly, this gives lots of errors, which I already commented on. ./mach lint comm/chat/content/chat-account-richlistitem.js
Attachment #9049865 - Flags: review?(mkmelin+mozilla) → review-
Attached patch De-XBL_account.patch (obsolete) (deleted) — — Splinter Review
Attachment #9049865 - Attachment is obsolete: true
Attachment #9051753 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9051753 [details] [diff] [review] De-XBL_account.patch Review of attachment 9051753 [details] [diff] [review]: ----------------------------------------------------------------- The patch doesn't build (account.xml needs to be removed from the jar). But after fixing that it seems to work. ::: chat/content/chat-account-richlistitem.js @@ +9,5 @@ > +{ > + let { Services } = ChromeUtils.import("resource://gre/modules/Services.jsm"); > + let { DownloadUtils } = ChromeUtils.import("resource://gre/modules/DownloadUtils.jsm"); > + /** > + * The MozAccountRichlist widget displays the information about the ozChatAccountRichlistitem ::: mail/components/im/content/imAccounts.js @@ +277,4 @@ > this.disableTimerID = setTimeout(function(aItem) { > gAccountManager.disableTimerID = 0; > gAccountManager.disableCommandItems(); > + aItem.setFocus(); why this change?
Attachment #9051753 - Flags: review?(mkmelin+mozilla) → feedback+

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

The patch doesn't build (account.xml needs to be removed from the jar). But
after fixing that it seems to work.

What should I do to remove this error?

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

why this change?

Previously, buttons was another binding. And setFocus was a function in that binding. Now, we have combined both the bindings so we call setFocus directly.

(In reply to Khushil Mistry [:khushil324] from comment #13)

What should I do to remove this error?

Your patch needs to include removing account.xml from the packaging in relevant jar.mn file.

Attached patch De-XBL_account.patch (obsolete) (deleted) — — Splinter Review
Attachment #9051753 - Attachment is obsolete: true
Attachment #9052696 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9052696 [details] [diff] [review] De-XBL_account.patch Review of attachment 9052696 [details] [diff] [review]: ----------------------------------------------------------------- Looks all good, except the commit message format ;)
Attachment #9052696 - Flags: review?(mkmelin+mozilla) → review+

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

Looks all good, except the commit message format ;)

Yes, will change that in a while.

Attached patch De-XBL_account.patch (obsolete) (deleted) — — Splinter Review
Attachment #9052696 - Attachment is obsolete: true
Attachment #9052849 - Flags: review+
Attachment #9052849 - Flags: feedback?(mkmelin+mozilla)
Comment on attachment 9052849 [details] [diff] [review] De-XBL_account.patch Review of attachment 9052849 [details] [diff] [review]: ----------------------------------------------------------------- s/converted/convert You could also have added ", r=mkmelin" while you were at it
Attachment #9052849 - Flags: feedback?(mkmelin+mozilla)
Attached patch De-XBL_account.patch (deleted) — — Splinter Review
Attachment #9052849 - Attachment is obsolete: true
Keywords: checkin-needed

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/34cf7d8564ad
[de-xbl] convert account and buttons bindings to custom element. r=mkmelin

Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED

Please include the word "binding/bindings" in the commit message. The second binding was called buttons.

Target Milestone: --- → Thunderbird 68.0
Type: enhancement → task
Regressions: 1581152
Regressions: 1591506
Regressions: 1591505
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: