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

Status: RESOLVED FIXED

(Thunderbird :: Instant Messaging, task)

Comment 1

[Khushil Mistry [:khushil324]]

Attachment: De-XBL_account.patch

Comment 2

[Magnus Melin [:mkmelin]]

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

Comment 3

[Khushil Mistry [:khushil324]]

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

Comment 4

[Khushil Mistry [:khushil324]]

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

Comment 5

[Khushil Mistry [:khushil324]]

Attachment: De-XBL_account.patch

Comment 6

[Magnus Melin [:mkmelin]]

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

Comment 7

[Khushil Mistry [:khushil324]]

Attachment: De-XBL_account.patch

Comment 8

[Magnus Melin [:mkmelin]]

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

Comment 9

[Khushil Mistry [:khushil324]]

Attachment: De-XBL_account.patch

Comment 10

[Magnus Melin [:mkmelin]]

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

Comment 11

[Khushil Mistry [:khushil324]]

Attachment: De-XBL_account.patch

Comment 12

[Magnus Melin [:mkmelin]]

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?

Comment 13

[Khushil Mistry [:khushil324]]

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

Comment 14

[Khushil Mistry [:khushil324]]

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

Comment 15

[Magnus Melin [:mkmelin]]

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

Comment 16

[Khushil Mistry [:khushil324]]

Attachment: De-XBL_account.patch

Comment 17

[Magnus Melin [:mkmelin]]

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

Comment 18

[Khushil Mistry [:khushil324]]

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

Looks all good, except the commit message format ;)

Yes, will change that in a while.

Comment 19

[Khushil Mistry [:khushil324]]

Attachment: De-XBL_account.patch

Comment 20

[Magnus Melin [:mkmelin]]

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

Comment 21

[Khushil Mistry [:khushil324]]

Attachment: De-XBL_account.patch

Comment 22

[Khushil Mistry [:khushil324]]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=6d088f0d73dbaebf4a1497c459043a919b783c60

Comment 23

[Pulsebot]

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

Comment 24

[Jorg K (CEST = GMT+2)]

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