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

RESOLVED FIXED in Thunderbird 68.0

Status

enhancement
RESOLVED FIXED
2 months ago
27 days ago

People

(Reporter: khushil324, Assigned: khushil324)

Tracking

(Blocks 1 bug)

Trunk
Thunderbird 68.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 7 obsolete attachments)

Comment hidden (empty)
(Assignee)

Updated

2 months ago
Component: General → Instant Messaging
(Assignee)

Updated

2 months ago
Assignee: nobody → khushil324
Status: NEW → ASSIGNED
(Assignee)

Comment 1

2 months ago
Posted patch De-XBL_account.patch (obsolete) — 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)
(Assignee)

Comment 3

2 months ago

(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?

(Assignee)

Comment 4

2 months ago

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

(Assignee)

Comment 5

2 months ago
Posted patch De-XBL_account.patch (obsolete) — 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
(Assignee)

Comment 7

a month ago
Posted patch De-XBL_account.patch (obsolete) — 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)
(Assignee)

Comment 9

a month ago
Posted patch De-XBL_account.patch (obsolete) — 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-
(Assignee)

Comment 11

a month ago
Posted patch De-XBL_account.patch (obsolete) — 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+
(Assignee)

Comment 13

29 days ago

(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?

(Assignee)

Comment 14

29 days ago

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

(Assignee)

Comment 16

28 days ago
Posted patch De-XBL_account.patch (obsolete) — 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+
(Assignee)

Comment 18

28 days ago

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

Looks all good, except the commit message format ;)

Yes, will change that in a while.

(Assignee)

Comment 19

28 days ago
Posted patch De-XBL_account.patch (obsolete) — 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)
(Assignee)

Comment 21

28 days ago
Attachment #9052849 - Attachment is obsolete: true
(Assignee)

Updated

27 days ago
Keywords: checkin-needed

Comment 23

27 days ago

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
Last Resolved: 27 days ago
Keywords: checkin-needed
Resolution: --- → FIXED

Comment 24

27 days ago

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

Target Milestone: --- → Thunderbird 68.0
You need to log in before you can comment on or make changes to this bug.