1525589 - [de-xbl] convert download to custom element

Status: RESOLVED FIXED

(Thunderbird :: General, task)

Comment 1

[Khushil Mistry [:khushil324]]

Attachment: De-XBL_richlist-download.patch

Comment 2

[Khushil Mistry [:khushil324]]

I was facing CSS issue with the image of the downloaded item. Height was not getting set up properly(32px here). It was taking the height of the whole container so the image was getting skewed. So I have used max-height CSS attribute here.

Comment 3

[Magnus Melin [:mkmelin]]

Comment on attachment 9043711 [details] [diff] [review]
De-XBL_richlist-download.patch

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

::: mail/components/downloads/content/download.js
@@ +3,5 @@
> +  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +/**
> + * The MozDownloadView widget displays information about the downloaded file:

this isn't a really a view, it's an item in a view

@@ +6,5 @@
> +/**
> + * The MozDownloadView widget displays information about the downloaded file:
> + * e.g. downloaded file type, name, size, date and sender's name.
> + * It is shown in the Saved Files view as an item of the downloaded items list.
> + */

this documentation needs to be directly before the class definition

@@ +49,5 @@
> +    this.appendChild(this._vbox);
> +    this.appendChild(this._sender);
> +
> +    this._updateAttributes();
> +  }

I'd think it's enough to do this._updateAttributes() in the connectedCallback, no?

@@ +83,5 @@
> +MozXULElement.implementCustomInterface(
> +  MozDownloadView,
> +  [Ci.nsIDOMXULSelectControlItemElement]
> +);
> +

prefer this formatting:

MozXULElement.implementCustomInterface(
  MozDownloadView,  [Ci.nsIDOMXULSelectControlItemElement]
);

@@ +84,5 @@
> +  MozDownloadView,
> +  [Ci.nsIDOMXULSelectControlItemElement]
> +);
> +
> +customElements.define("download-view", MozDownloadView, {

I'd suggest downloades-view-item

@@ +86,5 @@
> +);
> +
> +customElements.define("download-view", MozDownloadView, {
> +  extends: "richlistitem",
> +});

for easier greppability, you can put stuff things like these that would only slightly go over 80ch on to one line, as

customElements.define("download-view", MozDownloadView, { extends: "richlistitem"} );

::: mail/themes/linux/mail/downloads/aboutDownloads.css
@@ +24,4 @@
>  }
>  
>  #msgDownloadsRichListBox > richlistitem.download {
> +  display: -webkit-inline-box;

we don't put -webbit css properties into internal code, ever

It's apparently a deprecated property anyway, you should use something from the recent flexbox spec if you need it.

Comment 4

[Magnus Melin [:mkmelin]]

And how did the investigation go as for the feasibility of using what toolkit did in bug 1452626?

Comment 5

[Khushil Mistry [:khushil324]]

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

And how did the investigation go as for the feasibility of using what toolkit did in bug 1452626?

I tried it. I understood how we can do it. It will take little time for me to code. But I thought these components are isolated and fairly straight forward in terms of implementation. so we can go with CE and try to implement toolkit methods in some other components. What do you think?

Comment 6

[Magnus Melin [:mkmelin]]

Sure, let's go with the CE approach.

Comment 7

[Khushil Mistry [:khushil324]]

Attachment: De-XBL_richlist-download.patch

Comment 8

[Magnus Melin [:mkmelin]]

Comment on attachment 9044011 [details] [diff] [review]
De-XBL_richlist-download.patch

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

Looks good. r=mkmelin with the below addressed

::: mail/components/downloads/content/aboutDownloads.xul
@@ +23,5 @@
>            src="chrome://global/content/globalOverlay.js"/>
>    <script type="application/javascript"
>            src="chrome://messenger/content/downloads/aboutDownloads.js"/>
> +  <script type="application/javascript"
> +          src="chrome://messenger/content/downloads/download.js"/>

please make the file also be named download-view-item.js

::: mail/components/downloads/content/download.js
@@ +3,5 @@
> +  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +
> +/* global MozElements, MozXULElement, Ci */

Ci is in the global ignore list for mozilla

Comment 9

[Khushil Mistry [:khushil324]]

Attachment: De-XBL_richlist-download.patch

Comment 10

[Magnus Melin [:mkmelin]]

Please still change MozDownloadViewItem to MozDownloadsViewItem, and the download-view-item.js to downloads-view-item.js

It's downloads with an s. A view of downloads.

Comment 11

[Khushil Mistry [:khushil324]]

Attachment: De-XBL_richlist-download.patch

Comment 12

[Jorg K (CEST = GMT+2)]

Shouldn't you do a try run here? We have tests for downloads.

Comment 13

[Magnus Melin [:mkmelin]]

Sent one off - https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=f490bf6c95b8dc90e8133a9f4a713d43a358ca0d

Comment 14

[Magnus Melin [:mkmelin]]

https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=228952387&repo=try-comm-central&lineNumber=1802
TEST-UNEXPECTED-FAIL | /builds/worker/workspace/build/tests/mozmill/downloads/test-about-downloads.js | test-about-downloads.js::test_remove_file
TEST-UNEXPECTED-FAIL | /builds/worker/workspace/build/tests/mozmill/downloads/test-about-downloads.js | test-about-downloads.js::test_remove_multiple_files

Comment 15

[Magnus Melin [:mkmelin]]

I'm not sure this works now: https://searchfox.org/comm-central/source/mail/components/downloads/content/aboutDownloads.js#237

Also make sure firstElement is what's expected: https://searchfox.org/comm-central/source/mail/test/mozmill/downloads/test-about-downloads.js#197

So to debug, go to your obj-dir, then run
make SOLO_TEST=downloads/test-about-downloads.js mozmill-one

Comment 16

[Khushil Mistry [:khushil324]]

Attachment: De-XBL_richlist-download.patch

I found that it was an issue related to accessing firstElement. I have updated the test accordingly.

Comment 17

[Magnus Melin [:mkmelin]]

Comment on attachment 9046112 [details] [diff] [review]
De-XBL_richlist-download.patch

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

Verified the test works locally. r=mkmelin

Comment 18

[Magnus Melin [:mkmelin]]

Comment on attachment 9046112 [details] [diff] [review]
De-XBL_richlist-download.patch

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

For future bugs, please add a " - " after the bug number in the commit message. Like

Bug 1525589 - convert download binding to custom element downloads-view-item. r=mkmelin

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/50e9f0078bdd
De-XBL: convert download binding to custom element. r=mkmelin

Comment 20

[Tim Nguyen :ntim]

Since there's only one usage, you could have built the markup directly into the only callsite instead of creating a whole custom element.

See bug 1520924 and bug 1522279 for some examples how to do that.

Comment 21

[Magnus Melin [:mkmelin]]

Thanks Tim, agreed that could have been a good alternative. I'm not sure it matters much especially as this is only loaded for the downloads tab.

Khushil, can you still file a follow-up bug to directly build the markup instead? It looks like it would be fairly easy to move over.

Comment 22

[Khushil Mistry [:khushil324]]

Sure, I have submitted the new bug: Bug 1537934. Will post the updates there.