1528214 - [de-xbl] convert activity-group to custom element

Status: RESOLVED FIXED

(Thunderbird :: General, task)

Comment 1

[Khushil Mistry [:khushil324]]

Attachment: De-XBL_activity-group.patch

Comment 2

[Magnus Melin [:mkmelin]]

Comment on attachment 9044729 [details] [diff] [review]
De-XBL_activity-group.patch

Review of attachment 9044729 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/components/activity/content/activity-group.js
@@ +7,5 @@
> +/* global MozElements, MozXULElement */
> +
> +/**
> + * The MozActivityGroup widget displays information about the activities of
> + * the group: e.g. Name of the group, list of the activities with their name,

name

@@ +10,5 @@
> + * The MozActivityGroup widget displays information about the activities of
> + * the group: e.g. Name of the group, list of the activities with their name,
> + * progress and icon. It is shown in Activity manager window. It gets removed
> + * when there is no activities from the group.
> + * @extends MozElements.MozRichlistitem

{MozElements.MozRichlistitem}

@@ +14,5 @@
> + * @extends MozElements.MozRichlistitem
> + */
> +class MozActivityGroup extends MozElements.MozRichlistitem {
> +  static get observedAttributes() {
> +    return ["contextDisplayText", "retry_enabled"];

retry_enabled is never used by anyone, so let's remove that

@@ +27,5 @@
> +            <vbox pack="start">
> +              <label crop="left" class="contextDisplayText"></label>
> +            </vbox>
> +            <vbox pack="center" flex="2">
> +              <button class="retry mini-button" tooltiptext="FROM-DTD-cmd-retry-label"

when you get these FROM-DTD, you neeed to fix that manually

MozXULElement.parseXULToFragment takes the url where to find the dtds as a second argument.

@@ +28,5 @@
> +              <label crop="left" class="contextDisplayText"></label>
> +            </vbox>
> +            <vbox pack="center" flex="2">
> +              <button class="retry mini-button" tooltiptext="FROM-DTD-cmd-retry-label"
> +                cmd="cmd_retry" ondblclick="event.stopPropagation();" oncommand="retry();"></button>

does this really work, the event.stopPropagation() ?

@@ +60,5 @@
> +    _label.setAttribute("tooltiptext", this.getAttribute("contextDisplayText"));
> +
> +    let _button = this.querySelector(".retry");
> +    _button.setAttribute("enabled", this.getAttribute("retry_enabled"));
> +  }

would be preferable to switch to the new way that requires less boilerplate:

static get inheritedAttributes() {

@@ +71,5 @@
> +    return this.querySelector(".activitygroupbox");
> +  }
> +
> +  retry() {
> +    /* globals activityManager */

please put that at the top of the file
inside code, we also just use // comments

@@ +75,5 @@
> +    /* globals activityManager */
> +    let processes = activityManager.getProcessesByContext(this.contextType,
> +      this.contextObj, {});
> +    for (let process of processes) {
> +      if (process.retryHandler)

please always  { } even if just one line

::: mail/components/activity/content/activity.js
@@ +112,4 @@
>  
>        if (group) {
>          // get the inner list element of the group
> +        let groupView = document.querySelector(".activitygroupbox");

probably group.querySelector instead?

@@ +157,2 @@
>          if (actbinding) {
> +          let groupView = document.querySelector(".activitygroupbox");

not document.querySelector, probably group here too (can't see the context)

Comment 3

[Khushil Mistry [:khushil324]]

Attachment: De-XBL_activity-group.patch

Comment 4

[Magnus Melin [:mkmelin]]

Comment on attachment 9045003 [details] [diff] [review]
De-XBL_activity-group.patch

Review of attachment 9045003 [details] [diff] [review]:
-----------------------------------------------------------------

Clicking an item (Deleted 4 messags from Inbox) after opening the Activity Manager (possibly only while there is only one item in the list), I get

JavaScript error: chrome://global/content/elements/richlistbox.js, line 397: TypeError: aItem is null

    removeItemFromSelection(aItem)
-->    if (!aItem.selected) {



When I move a message to another folder, I got this:

2019-02-20 12:15:41	nsActivityManager	ERROR	Exception calling onAddedActivity[Exception... "[JavaScript Error: "groupView is null" {file: "chrome://messenger/content/activity.js" line: 116}]'[JavaScript Error: "groupView is null" {file: "chrome://messenger/content/activity.js" line: 116}]' when calling method: [nsIActivityMgrListener::onAddedActivity]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: file:///home/magnus/Code/tb/mozilla/obj-x86_64-pc-linux-gnu/dist/bin/components/nsActivityManager.js :: addActivity :: line 65"  data: yes]
2019-02-20 12:15:41	activitymgr	ERROR	addActivityBinding: TypeError: groupView is null


The patch looks basically ok. Perhaps there are just now problems because the related elements aren't yet converted?

Comment 5

[Khushil Mistry [:khushil324]]

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

Clicking an item (Deleted 4 messags from Inbox) after opening the Activity
Manager (possibly only while there is only one item in the list), I get

JavaScript error: chrome://global/content/elements/richlistbox.js, line 397:
TypeError: aItem is null

removeItemFromSelection(aItem)

--> if (!aItem.selected) {

I found this issue on trunk. It was behaving correctly so I am not sure what to do exactly so I have kept it for the end after converting related activity bindings.

When I move a message to another folder, I got this:

2019-02-20 12:15:41 nsActivityManager ERROR Exception calling
onAddedActivity[Exception... "[JavaScript Error: "groupView is null" {file:
"chrome://messenger/content/activity.js" line: 116}]'[JavaScript Error:
"groupView is null" {file: "chrome://messenger/content/activity.js" line:
116}]' when calling method: [nsIActivityMgrListener::onAddedActivity]"
nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"
location: "JS frame ::
file:///home/magnus/Code/tb/mozilla/obj-x86_64-pc-linux-gnu/dist/bin/
components/nsActivityManager.js :: addActivity :: line 65" data: yes]
2019-02-20 12:15:41 activitymgr ERROR addActivityBinding: TypeError:
groupView is null

The patch looks basically ok. Perhaps there are just now problems because
the related elements aren't yet converted?

I will look into this now.

Comment 6

[Magnus Melin [:mkmelin]]

Comment on attachment 9045003 [details] [diff] [review]
De-XBL_activity-group.patch

Review of attachment 9045003 [details] [diff] [review]:
-----------------------------------------------------------------

Retrying this I can't reproduce the error.

Can you however, update to use static get inheritedAttributes instead. THen you don't need get observedAttribute, nor attributeChangedCallback. In connectedCallack you call this.initializeAttributeInheritance();

::: mail/components/activity/content/activity-group.js
@@ +29,5 @@
> +            </vbox>
> +            <vbox pack="center" flex="2">
> +              <button class="retry mini-button" tooltiptext="&cmd.retry.label;"
> +                cmd="cmd_retry" ondblclick="event.stopPropagation();" oncommand="retry();"></button>
> +            </vbox>

I don't think this is actually ever used. probably never worked. you can remove the whole vbox and all related items (like the dtd)

Comment 7

[Khushil Mistry [:khushil324]]

Attachment: De-XBL_activity-group.patch

Any suggestions on how to use static get inheritedAttributes() effectively?

Comment 8

[Magnus Melin [:mkmelin]]

Comment on attachment 9045764 [details] [diff] [review]
De-XBL_activity-group.patch

Review of attachment 9045764 [details] [diff] [review]:
-----------------------------------------------------------------

I think that is the correct way to use it.

LGTM, r=mkmelin

Sent to try now: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=5b91f36ba7905d07cad011730399231b9dd9073c

Comment 9

[Khushil Mistry [:khushil324]]

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

I think that is the correct way to use it.

LGTM, r=mkmelin

Do I need to add r=mkmelin in the commit message or will it be taken care at the time of check-in ?

Comment 10

[Magnus Melin [:mkmelin]]

Comment on attachment 9045764 [details] [diff] [review]
De-XBL_activity-group.patch

Review of attachment 9045764 [details] [diff] [review]:
-----------------------------------------------------------------

Someone would add it for you. But, please fix this one thing I noticed:

::: mail/components/activity/content/activity-group.js
@@ +39,5 @@
> +
> +    this.contextObj = null;
> +  }
> +
> +  connectedCallback() {

Please add the super.connectedCallback() too here

Comment 11

[Magnus Melin [:mkmelin]]

The try run is good, the failures present are expected.

Comment 12

[Khushil Mistry [:khushil324]]

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

Please add the super.connectedCallback() too here

It is showing an error that super.connectedCallback is not a function.

Comment 13

[Magnus Melin [:mkmelin]]

Ok. Ready to land then.

Comment 14

[Jorg K (CEST = GMT+2)]

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

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/01b83ea83a23
De-XBL: convert activity-group to custom element. r=mkmelin DONTBUILD