Closed Bug 1528214 Opened 6 years ago Closed 6 years ago

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

Categories

(Thunderbird :: General, task)

Product:
Thunderbird
Component:
General
Type:
task
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 67.0

People

(Reporter: khushil324, Assigned: khushil324)

References

Details

Attachments

(1 file, 2 obsolete files)

De-XBL_activity-group.patch
(deleted), patch
mkmelin
: review+
Details | Diff | Splinter Review
No description provided.
Assignee: nobody → khushil324
Status: NEW → ASSIGNED
Blocks: tb-war-on-xbl
Attached patch De-XBL_activity-group.patch (obsolete) (deleted) — — Splinter Review
Attachment #9044729 - Flags: review?(mkmelin+mozilla)
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)
Attachment #9044729 - Flags: review?(mkmelin+mozilla) → review-
Attached patch De-XBL_activity-group.patch (obsolete) (deleted) — — Splinter Review
Attachment #9044729 - Attachment is obsolete: true
Attachment #9045003 - Flags: review?(mkmelin+mozilla)
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?

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

Attachment #9045003 - Flags: review?(mkmelin+mozilla)
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)
Attached patch De-XBL_activity-group.patch (deleted) — — Splinter Review

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

Attachment #9045003 - Attachment is obsolete: true
Attachment #9045764 - Flags: review?(mkmelin+mozilla)
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
Attachment #9045764 - Flags: review?(mkmelin+mozilla) → review+

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

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

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

Ok. Ready to land then.

Keywords: checkin-needed

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/01b83ea83a23
De-XBL: convert activity-group to custom element. r=mkmelin DONTBUILD

Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 67.0
Type: defect → task
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: