801138 - Work - Show downloaded files

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: General, defect)

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

We need to define what more needs to be displayed.

Comment 2

[Sam Foster [:sfoster] (he/him)]

We do need more definition on which each item in the list should contain. 
In desktop we have icon, filename, filesize, source domain, downloaded when. So far we just show the icon and the filename here. That is likely a separate ticket though. Download items do get added as the download begins, and the list of previously downloads is complete both on the start and downloads view. 

There is a bug in the display of the download item when it is first created - the label is always '0'. I'm treating that as the blocker prevent this from being complete.

Comment 3

[Sam Foster [:sfoster] (he/him)]

Attachment: change richgrid insertItemAt to expect index first

Confusion about the signature of richgrid's insertItemAt caused this bug. All the other widgets seem to expect index first in this kind of method, so I'm proposing this API change. The patch includes the corresponding change to richgrid insertIndexAt calls in bookmarks.js

Comment 4

[Matt Brubeck (:mbrubeck)]

http://hg.mozilla.org/projects/elm/rev/11065a519c17

Removing uiwanted keyword; we can open new bugs for any UX changes needed here.

Comment 5

[Sam Foster [:sfoster] (he/him)]

To make use of the richgrid, each download item needs to be a richgriditem. There are pre-existing bindings for the various item states which need minor work to hook up.

Other tickets cover the specifics of the download behavior and UI, but I think this ticket implies at least a test that verifies the UI accurately reflects the download DB.

Comment 6

[Sam Foster [:sfoster] (he/him)]

Attachment: WIP on downloads richgrid item/bindings and test

This is work-in-progress, just want to check I'm going down the right path - both in what the scope of this ticket is and how I'm going about it.

Comment 7

[Matt Brubeck (:mbrubeck)]

Comment on attachment 685224 [details] [diff] [review]
WIP on downloads richgrid item/bindings and test

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

Looks good to me.

Comment 8

[Sam Foster [:sfoster] (he/him)]

Attachment: Extend richgrid-item for the dowload item bindings. Fix errors arising from download-item (element) vs download object argument. New DownloadReady event fired when the last item is added

This patch gets us extending richgrid-item for the download items, with stubs (at least) for the progress bar, open, delete, pause and cancel button. The functionality for these is covered under seperate tickets so maybe this gets us far enough for this ticket?

Comment 9

[Matt Brubeck (:mbrubeck)]

Comment on attachment 688824 [details] [diff] [review]
Extend richgrid-item for the dowload item bindings. Fix errors arising from download-item (element) vs download object argument. New DownloadReady event fired when the last item is added

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

Looks good to me.  I didn't go through the test code with a fine-toothed comb, but it looks reasonable (assuming it runs and passes).

::: browser/metro/base/content/downloads.js
@@ +175,5 @@
>    Downloads.manager.addListener(this._progress);
>  
> +  // this._set.addEventListener("DownloadsReady", function(aEvent) {
> +  //   dump("*** DownloadsReady **** \n");
> +  // }.bind(this), true);

Reminder to remove this debug code before checkin.

Comment 10

[Sam Foster [:sfoster] (he/him)]

Attachment: Revised patch to define downloads as richgrid-items with associated actions

I revised the last patch (688824) to:
* fix the issue with the label on new downloads
* remove commented DownloadsReady event listener (debug code)
* pare down the tests (which were broken and WIP) to a basic UI smoke test

I'm continuing work on the tests, but with your approval this patch can land as-is. It makes open downloads, download progress WFM, and unblocks a patch in my queue to hook up download removal.

Comment 11

[Matt Brubeck (:mbrubeck)]

Comment on attachment 689507 [details] [diff] [review]
Revised patch to define downloads as richgrid-items with associated actions

Thanks!  I'll check this in today.

Comment 12

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/projects/elm/rev/e46eaded923c

Comment 13

[Sam Foster [:sfoster] (he/him)]

Attachment: Chrome tests for the show-downloads UI, functionality

Tests use the Tasks and Promises module to line up fairly complex, async test dependencies and sequence. The downloads db emptying and re-population with known data was adapted from browser/component/downloads test's head.js. This seems like a lot of work? Perhaps the tests can do less to offer the same guarantees, or some better mocking is possible. 

Further tests will follow (on the relevant tickets) to check the process, add, remove etc. functionality offered by the UI.

Comment 14

[Matt Brubeck (:mbrubeck)]

Comment on attachment 693864 [details] [diff] [review]
Chrome tests for the show-downloads UI, functionality

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

This looks great!  Thanks for trying out the Task.js stuff.  I just have some minor style nits, and some areas where I think we can improve the clarity.

::: browser/metro/base/tests/browser_downloads.js
@@ +2,5 @@
>  /* vim: set ts=2 et sw=2 tw=80: */
>  /* Any copyright is dedicated to the Public Domain.
>     http://creativecommons.org/publicdomain/zero/1.0/ */
>  
> +"use strict"

Nit: Missing semicolon.

@@ +15,5 @@
>  
> +XPCOMUtils.defineLazyModuleGetter(this, "Promise",
> +                                  "resource://gre/modules/commonjs/promise/core.js");
> +XPCOMUtils.defineLazyModuleGetter(this, "Task",
> +                                  "resource://gre/modules/Task.jsm");

I would eventually like to move these imports into our head.js, maybe along with utility functions like equalStrings, but this can wait until we actually start reusing them in other tests.  (For example, it would be nice to convert addTab to use Task.js.)

@@ +31,5 @@
> +
> +function equalStrings(){
> +  let ref = ""+arguments[0];
> +  for(let i=1; i<arguments.length; i++){
> +    if(ref !== ""+arguments[i]) {

Nit: Add a space after keywords like "for" or "if".

@@ +239,5 @@
> +
> +gTests.push({
> +  desc: "UI sanity check",
> +  run: function(){
> +    if (false) yield;

I don't like this.  Is there a way we can avoid needing incantations like this in individual tests?  Maybe the main test() function can check whether "run()" returns an iterator, so it can handle both regular functions and generator functions?  Or we could use a different attribute name (like "test" instead of "run") for test cases that don't need to spawn a task, just to make this more explicit.

@@ +263,5 @@
> +    // .. fail a test if the timeout is exceeded
> +    var readyTimer = setTimeout(function(){
> +      ok(false, "DownloadsReady event never fired");
> +      readyDeferred.reject( new Error("DownloadsReady event timeout") );
> +    }, 2000);

I think we should extract this pattern (wait for an event, with a timeout) into a helper function that could look something like:

  let deferred = waitForEvent(downloadsList, "DownloadsReady", 2000);
  DownloadsPanelView._view.getDownloads();
  yield deferred.promise;

@@ +268,5 @@
> +
> +    downloadslist.addEventListener("DownloadsReady", function(){
> +      clearTimeout(readyTimer);
> +      readyDeferred.resolve(true);
> +    }, false);

I think we need to remove the event listener after it fires (or times out).

Comment 15

[Sam Foster [:sfoster] (he/him)]

Attachment: Revised chrome tests for the show-downloads UI, functionality

I've addressed the style issues and moved that duplicated DownloadsReady event code into a waitForEvent helper - which removes the listener as its first business in either the callback or errback. 

Yes, some of the helpers and infrastructure here eventually belong in head.js. I wanted to limit the impact of this patch initially, and refactor when there was a need for this stuff in other tests.

Comment 16

[Matt Brubeck (:mbrubeck)]

Comment on attachment 694149 [details] [diff] [review]
Revised chrome tests for the show-downloads UI, functionality

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

Looks good!  I'll fix the last nits before checkin.

::: browser/metro/base/tests/browser_downloads.js
@@ +289,5 @@
> +
> +    yield isReady;
> +
> +    if (!isReady || isReady instanceof Error){
> +      is(false, "DownloadsReady event never fired");

Should be "ok(false, ...)"

@@ +341,5 @@
> +
> +      yield isReady;
> +
> +      if (!isReady || isReady instanceof Error){
> +        is(false, "DownloadsReady event never fired");

ditto

Comment 17

[Matt Brubeck (:mbrubeck)]

Comment on attachment 694149 [details] [diff] [review]
Revised chrome tests for the show-downloads UI, functionality

https://hg.mozilla.org/projects/elm/rev/e8ffe4076eed

Comment 18

[Matt Brubeck (:mbrubeck)]

Resolving bugs in the Firefox for Metro product that are fixed on the elm branch.  Sorry for the bugspam.  Search your email for "bugspam-elm" if you want to find and delete all of these messages at once.