883951 - Initiate a download request, keep track of its progress, and show downloads completing using an Infobar

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: Downloads, defect)

Comment 1

[Marina Samuel [:emtwo]]

Attachment: WIP: Download Request Infobar

So far:
* Run, Save, Cancel buttons work according to their basic functionality
* The file name, size and host is shown in the bar

What else:
* Bar should be at bottom
* Bold the file name and host

Comment 2

[Marina Samuel [:emtwo]]

Attachment: Screenshot of WIP

Comment 3

[Marina Samuel [:emtwo]]

Just noticed, we also don't want the 'X' on the right of the bar in the case of a download request.

Comment 4

[Marina Samuel [:emtwo]]

Comment on attachment 763867 [details] [diff] [review]
WIP: Download Request Infobar

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

::: browser/metro/base/content/downloads.js
@@ +38,5 @@
>    },
>  
>    uninit: function dh_uninit() {
>      Services.obs.removeObserver(this, "dl-start");
>      Services.obs.removeObserver(this, "dl-done");

Should also remove "dl-run" here

::: browser/metro/components/HelperAppDialog.js
@@ +53,5 @@
> +                            .QueryInterface(Ci.nsIInterfaceRequestor)
> +                            .getInterface(Ci.nsIDOMWindow)
> +                            .QueryInterface(Ci.nsIDOMChromeWindow);
> +     return chromeWin;
> +  },

This code is identical to code in ContentPermissionPrompt.js but wasn't sure the right place to put it so it can be shared. Advice appreciated :)

@@ +55,5 @@
> +                            .QueryInterface(Ci.nsIDOMChromeWindow);
> +     return chromeWin;
> +  },
> +
> +  _showDownloadInfobar: function do_showDownloadInfobar(aLauncher) {

Kind of a chunky function. Maybe break it down a bit more.

@@ +95,5 @@
> +    let chromeWin = this._getChromeWindow(window).wrappedJSObject;
> +    var notificationBox = chromeWin.Browser.getNotificationBox();
> +    downloadSize = this._getDownloadSize(aLauncher.contentLength);
> +
> +    let msg = browserBundle.formatStringFromName("alertDownloadSave", [aLauncher.suggestedFileName, downloadSize, aLauncher.source.host], 3);

Split this into two lines

Comment 5

[Allison Naaktgeboren :ally]

Comment on attachment 763867 [details] [diff] [review]
WIP: Download Request Infobar

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

f+! It's moving in the right direction. Good job so far :)

You'll also need to keep an eye on the tests, and modify those to support the new world order, such as http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/tests/mochitest/browser_downloads.js. We can opt to do that in a followup patch and/or bug as needed.

I see no css in this patch, which is almost certainly what you'll need to manipulate the bar's layout & location. A brief skim through metro suggests that you'll want to start looking athttp://mxr.mozilla.org/mozilla-central/source/browser/metro/theme/platform.css#389

The bolding of filename/host might be a bit trickier since that's done by browserBundle.formatStringFromName(). I have high hopes of controlling that through css, but that may be easily doable.

Actual review should be done by :fryn

::: browser/metro/base/content/bindings/downloads.xml
@@ +176,5 @@
>            <xul:button anonid="showpage-button" label="&downloadShowPage.label;"
>                        oncommand="Downloads.showPage(document.getBindingParent(this));"/>
>            <xul:spacer flex="1"/>
>            <xul:button anonid="open-button" label="&downloadOpen2.label;"
> +                      oncommand="Downloads.openXULDownload(document.getBindingParent(this));"/>

nit: I think the name should describe the behavior it serves, not so much the technology we do it in. I see why you called it that, so if we cant come up with a better name, it can stick around.

::: browser/metro/base/content/browser-scripts.js
@@ -115,5 @@
>    ["Bookmarks", "chrome://browser/content/bookmarks.js"],
>    ["Downloads", "chrome://browser/content/downloads.js"],
>    ["BookmarksPanelView", "chrome://browser/content/bookmarks.js"],
>    ["ConsolePanelView", "chrome://browser/content/console.js"],
>    ["DownloadsPanelView", "chrome://browser/content/downloads.js"],

My understanding is that the DownloadsPanelView is not part of the new UI & should be ripped out of the xul & js. Are you planning to do that on a separate bug?

::: browser/metro/base/content/downloads.js
@@ +33,5 @@
>      this._inited = true;
>  
>      Services.obs.addObserver(this, "dl-start", true);
>      Services.obs.addObserver(this, "dl-done", true);
> +    Services.obs.addObserver(this, "dl-run", true);

Is there a reason there isn't a matching removeObserver() call? Possibly in the uninit() function?

::: browser/metro/components/HelperAppDialog.js
@@ +58,5 @@
> +
> +  _showDownloadInfobar: function do_showDownloadInfobar(aLauncher) {
> +    let browserBundle = Services.strings.createBundle("chrome://browser/locale/browser.properties");
> +
> +    var runButtonText =

Please use let, not var, in all new and modified code, especially within the same object or function. :)

Comment 6

[Marina Samuel [:emtwo]]

Attachment: v2 WIP: Download Request Infobar

Very minor changes, I think I just replaced "var" with "let". Will address other comments in a later comment.

Comment 7

[Marina Samuel [:emtwo]]

Attachment: v1 WIP: Show download progress (for 1 and multiple downloads) and download complete in infobar

Comment 8

[Marina Samuel [:emtwo]]

Attachment: Download Request Screenshot

Comment 9

[Marina Samuel [:emtwo]]

Attachment: Single Download Progress Screenshot

Comment 10

[Marina Samuel [:emtwo]]

Attachment: Multiple Download Progress Screenshot

Comment 11

[Marina Samuel [:emtwo]]

Attachment: Download Complete Screenshot

Comment 12

[Asa Dotzler [:asa]]

Looking great, Marina!

Comment 13

[Marina Samuel [:emtwo]]

In reply to comment 5:

I think I'll leave tests and ripping out code to another patch and/or bug. In terms of css, I will see what comments we have in today's meeting (I think mbrubeck was working on infobar changes) before revisiting

Comment 14

[Frank Yan (:fryn)]

Comment on attachment 766821 [details] [diff] [review]
v1 WIP: Show download progress (for 1 and multiple downloads) and download complete  in infobar

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

This doesn't apply cleanly to mozilla-central or mozilla-inbound.
Is it being built on top of another patch, or does it need to be updated?

Comment 15

[Marina Samuel [:emtwo]]

Frank, I think it should apply cleanly if you apply the patch "v2 WIP: Download Request Infobar" first. I should perhaps rename them better so the ordering is obvious. Sorry about that!

Comment 16

[Frank Yan (:fryn)]

(In reply to Marina Samuel [:emtwo] from comment #15)
> Frank, I think it should apply cleanly if you apply the patch "v2 WIP:
> Download Request Infobar" first. I should perhaps rename them better so the
> ordering is obvious. Sorry about that!

That worked; thanks! A "Part 1"/"Part 2" patch name prefix would be sufficient to clear up the confusion.

Comment 17

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

Comment on attachment 766821 [details] [diff] [review]
v1 WIP: Show download progress (for 1 and multiple downloads) and download complete  in infobar

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

I'm without a functioning build to try this out right now, so my comments are just looking at the code itself. Looking good so far, I eager to see it working.

::: browser/metro/base/content/Util.js
@@ +248,5 @@
>        Util.dumpLn("dumpDOMRect:", ex.message);
>      }
>    },
>  
> +  getDownloadSize: function dv__getDownloadSize (aSize) {

I looked at DownloadUtils.convertByteUnits and I now get it. You might add a comment here that convertByteUnits returns a [amount, localized-unit-string] array, hence the need for this helper.

::: browser/metro/base/content/downloads.js
@@ +12,5 @@
> +  _lastSec: Infinity,
> +  _notificationBox: null,
> +  _progressNotification: null,
> +  _notificationVisible: true,
> +  _progressNotificationInfo: [],

You use guids to key values in _progressNotificationInfo, looking at how you use this data below, I think Map is a better choice here

@@ +56,5 @@
>      let fileURI = aDownload.target;
>      let file = this._getLocalFile(fileURI);
>  
>      try {
> +      MetroUtils.launchInDesktop(aDownload.target.spec, "");

We've been seeing test failures around this code (unrelated to  your work). I would put the _getLocalFile call inside the try/catch. We can't recover meaningfully from the caught exception but maybe we can at least return false on failure and true on success?

@@ +67,5 @@
>      let id = aDownload.getAttribute("downloadId");
>      let download = this.manager.getDownload(id);
>  
>      if (download) {
>        // TODO <sfoster> add class/mark element for removal transition?

You can axe this comment, any removal transition will happen upstream in the view and CSS.

@@ +74,5 @@
>    },
>  
> +  // Cancels all downloads.
> +  cancelDownload: function dh_cancelDownload() {
> +    for (let guid in this._progressNotificationInfo) {

See above comment about the data structure used here. for/in implies an plain Object yet you assign an array.

@@ +186,5 @@
> +  },
> +
> +  _computeDownloadProgressString: function dv_computeDownloadProgressString(aDownload) {
> +    let totTransferred = 0, totSize = 0, totSecondsLeft = 0;
> +    for (var downloadGuid in this._progressNotificationInfo) {

nit: let
If you used a map you could for (let info of this._progressNotificationInfo) and save a lot of duplication here

Comment 18

[Frank Yan (:fryn)]

Comment on attachment 766821 [details] [diff] [review]
v1 WIP: Show download progress (for 1 and multiple downloads) and download complete  in infobar

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

::: browser/metro/base/content/Util.js
@@ +251,5 @@
>  
> +  getDownloadSize: function dv__getDownloadSize (aSize) {
> +    let displaySize = DownloadUtils.convertByteUnits(aSize);
> +    if (displaySize[0] > 0) // [0] is size, [1] is units
> +      return displaySize.join("");

You could do:
let [size, units] = DownloadUtils.convertByteUnits(aSize);
if (size[0] > 0)
  return size + units;

::: browser/metro/locales/en-US/chrome/browser.properties
@@ +36,5 @@
>  # Alerts
>  alertLinkBookmarked=Bookmark added
>  alertDownloads=Downloads
>  alertDownloadsStart=Downloading: %S
> +alertDownloadsStart2=Downlading %S, %S, %S

Remove the old versions of these strings if they are no longer used.
If they are still in use, then the newly added ones should have a different name.
Also, "downlading" typo :P

Comment 19

[Marina Samuel [:emtwo]]

Attachment: Part 1: v3: Download Request Infobar

Using the latest pull + clearer patch name

Comment 20

[Marina Samuel [:emtwo]]

Attachment: Part 2: v2: Show download progress (for 1 and multiple downloads) and download complete in infobar

* Using the latest pull + clearer patch name
* Addresses comment 17 and comment 18

Comment 21

[Marina Samuel [:emtwo]]

Attachment: Part 1: v4: Download Request Infobar

Had positive feedback on this one before. Minor changes since then. Just updating and aiming to ship this bug soon.

Comment 22

[Marina Samuel [:emtwo]]

Attachment: Part 2: v3: Show download progress (for 1 and multiple downloads) and download complete in infobar

Also had positive feedback on this one and minor changes afterwards.

Comment 23

[Marina Samuel [:emtwo]]

Attachment: Part 3: v1: Group download complete infobars for multiple downloads and handle run-after-download correctly

This one is new so may need more thorough review.

I've tested manually and it seems to be working well. I know ally pointed out earlier that downloads tests exist here: http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/tests/mochitest/browser_downloads.js

However, these mostly look related to DownloadsPanelView which I believe we're removing so do we think it makes sense to remove those tests?

Comment 24

[Marina Samuel [:emtwo]]

Attachment: Part 1: v4: Download Request Infobar

Oops that was not the right file!

Comment 25

[Matt Brubeck (:mbrubeck)]

Comment on attachment 776423 [details] [diff] [review]
Part 1: v4: Download Request Infobar

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

Looks good to me except for a potential race issue that I think should get fixed:

::: browser/metro/base/content/downloads.js
@@ +7,5 @@
>  
>  var Downloads = {
>    _inited: false,
>    _progressAlert: null,
> +  _runAfterDownload: false,

Since this is a global flag, it can get overwritten if you start a new download while a previous one is in progress.  I think we need something like a Map or WeakMap that stores a flag per download.

::: browser/metro/components/HelperAppDialog.js
@@ +38,5 @@
> +  },
> +
> +  _getDownloadSize: function dv__getDownloadSize (aSize) {
> +    let displaySize = DownloadUtils.convertByteUnits(aSize);
> +    if (displaySize[0] > 0) // [0] is size, [1] is units

You can use "let [size, unit] = DownloadUtils.convertByteUnits(aSize);" to make this a little more self-documenting.

Comment 26

[Matt Brubeck (:mbrubeck)]

Comment on attachment 776423 [details] [diff] [review]
Part 1: v4: Download Request Infobar

(In reply to Matt Brubeck (:mbrubeck) from comment #25)
> Since this is a global flag, it can get overwritten if you start a new
> download while a previous one is in progress.  I think we need something
> like a Map or WeakMap that stores a flag per download.

Never mind, I see this is fixed in the next patch.  Awesome!

Comment 27

[Matt Brubeck (:mbrubeck)]

Comment on attachment 776416 [details] [diff] [review]
Part 2: v3: Show download progress (for 1 and multiple downloads) and download complete in infobar

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

r=mbrubeck with the suggestions and nits below addressed.

::: browser/metro/base/content/bindings/downloads.xml
@@ +112,5 @@
>          <xul:hbox align="center">
>            <xul:progressmeter anonid="progressmeter" mode="normal" value="0" flex="1" xbl:inherits="value=progress,mode=progressmode"/>
>            <xul:button class="download-pause" label="&downloadPause.label;"
>                        oncommand="Downloads.pauseDownload(document.getBindingParent(this));"/>
> +          <xul:button class="download-cancel" label="&downloadCancel.label;"/>

Can we get rid of this whole file, along with the #downloads-container element in browser.xul and the DownloadsView object in downloads.js?

(If so, we can do that cleanup in a separate patch.)

::: browser/metro/base/content/downloads.js
@@ +45,5 @@
>      this._inited = true;
>  
>      Services.obs.addObserver(this, "dl-start", true);
>      Services.obs.addObserver(this, "dl-done", true);
>      Services.obs.addObserver(this, "dl-run", true);

What happens if a download fails?  Do we also need to listen dl-failed and messages from nsDownloadManager?
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/nsDownloadManager.cpp#2761

@@ +49,5 @@
>      Services.obs.addObserver(this, "dl-run", true);
> +
> +    let window = Services.wm.getMostRecentWindow("navigator:browser");
> +    let chromeWin = this._getChromeWindow(window).wrappedJSObject;
> +    this._notificationBox = chromeWin.Browser.getNotificationBox();

You can just use "let _notificationBox = Browser.getNotificationBox()" since this script is running in the chrome window.

@@ +109,5 @@
> +        Util.dumpLn("Failed to cancel download, with id: "+id+", download target URI spec: " + fileURI.spec);
> +        Util.dumpLn("Failed download source:"+(aDownload.source && aDownload.source.spec));
> +      }
> +
> +      this._progressNotificationInfo.delete(info[0]);

No need to delete items here since you reset the map right after the loop.

@@ +114,3 @@
>      }
>  
> +    this._progressNotificationInfo = new Map();

You can use .clear() here if you prefer, though either is fine with me.

You should clear _runDownloadBooleanMap too, I think.

@@ +173,5 @@
> +
> +  _getChromeWindow: function (aWindow) {
> +      let chromeWin = aWindow.QueryInterface(Ci.nsIInterfaceRequestor)
> +                            .getInterface(Ci.nsIWebNavigation)
> +                            .QueryInterface(Ci.nsIDocShellTreeItem)

I think this is unused if the suggestion above is applied.

@@ +315,5 @@
> +
> +        this._progressNotificationInfo.delete(download.guid);
> +        this._runDownloadBooleanMap.delete(download.targetFile.path);
> +        if (this._progressNotificationInfo.size == 0) {
> +          this._notificationBox.removeNotification(this._progressNotification);

You might want to set this._progressNotification = null too.

::: browser/metro/locales/en-US/chrome/browser.properties
@@ +30,5 @@
>  downloadsUnknownSize=Unknown size
>  downloadRun=Run
>  downloadSave=Save
>  downloadCancel=Cancel
> +downloadShowInFiles=Show in Files

This might deserve a comment since it is hard to understand if you aren't familiar with Windows 8.

@@ +35,5 @@
>  
>  # Alerts
>  alertLinkBookmarked=Bookmark added
>  alertDownloads=Downloads
> +alertDownloadsStart=Downloading %S, %S, %S

Please add a LOCALIZATION NOTE comment explaining what each %S represents.

We should also use new string IDs rather than change existing strings, for any changes that will require new translations (such as this one).  I'm not sure if this is *really* necessary for Metro strings right now since we disabled localization, but let's be safe just in case any previous translations are still out there.

@@ +41,5 @@
>  alertTapToSave=Tap to save this file.
>  alertDownloadsSize=Download too big
>  alertDownloadsNoSpace=Not enough storage space
>  alertDownloadSave=Do you want to run or save %S (%S) from %S?
> +alertDownloadMultiple=Downloading one file, #2, #3; Downloading #1 files, #2, #3

Also could use a LOCALIZATION NOTE.

Comment 28

[Matt Brubeck (:mbrubeck)]

Comment on attachment 776417 [details] [diff] [review]
Part 3: v1: Group download complete infobars for multiple downloads and handle run-after-download correctly

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

There's one bug and a few style issues noted below; I think they should be fairly straightforward to fix.

::: browser/metro/base/content/downloads.js
@@ +9,5 @@
>    _inited: false,
>    _progressAlert: null,
>  
>    _lastSec: Infinity,
> +  _downloadCount: 0,

I bit confused by _downloadCount at first, wondering why we don't just use the .size of the progress map.  I see now that it's meant to count all of the downloads that were ever tracked by the current notification bar, including completed ones, because that's the number we want to show.  Could you try to explain this a little more in a comment?

@@ +244,5 @@
>  
> +    // Keep track of a download count for displaying in the toast.
> +    if (this._progressNotificationInfo.size > this._downloadCount) {
> +      this._downloadCount = this._progressNotificationInfo.size;
> +    }

Sometimes this ends up with the wrong value for _downloadCount.  For example ("map" refers to Downloads._progressNotificationInfo):

 - Download A starts: map.size = 1, _downloadCount = 1
 - Download B starts: map.size = 2, _downloadCount = 2
 - Download B ends  : map.size = 1, _downloadCount = 2
 - Download C starts: map.size = 2, _downloadCount = 2
 - Download C ends  : map.size = 1, _downloadCount = 2
 - Download A ends  : map.size = 0, _downloadCount = 2

At this point, we say "2 files have been downloaded" but if I understand correctly, we want to say "3 files have been downloaded."

I think the correct logic is to increment _downloadCount whenever we receive a "dl-start" notification, and reset it to zero when map.size goes to zero.  (The reset part is already implemented correctly.)

@@ +326,5 @@
> +        }
> +
> +        if ((this._downloadCount > 1 && this._progressNotificationInfo.size == 1) ||
> +             (this._downloadCount == 1 && !runAfterDownload)) {
> +          // If we're done with all downloads, it's time for a toast/notification.

The condition in this if statement was a bit hard for me to decipher.  Can we move this code...

@@ +332,5 @@
>          }
>  
> +        if (this._progressNotificationInfo.size == 1) {
> +          this._downloadCount = 0;
> +        }

...and this code...

@@ +338,4 @@
>          this._progressNotificationInfo.delete(download.guid);
>          this._runDownloadBooleanMap.delete(download.targetFile.path);
>          if (this._progressNotificationInfo.size == 0) {
>            this._notificationBox.removeNotification(this._progressNotification);

...to here?  So it would be something like:

  if (this._progressNotificationInfo.size == 0) {
    // All the downloads are done.
    if (this._downloadCount > 1 || !runAfterDownload)) {
      this._showDownloadCompleteNotification(download);
    }
    this._downloadCount = 0;
    this._notificationBox.removeNotification(this._progressNotification);
  }

Comment 29

[Marina Samuel [:emtwo]]

Attachment: Part 3: v2: Group download complete infobars for multiple downloads and handle run-after-download correctly

Comment 30

[Marina Samuel [:emtwo]]

The above patch addresses issues in comment 28.

Comment 31

[Matt Brubeck (:mbrubeck)]

Comment on attachment 776648 [details] [diff] [review]
Part 3: v2: Group download complete infobars for multiple downloads and handle run-after-download correctly

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

Thanks!  Just one issue below that I missed in my previous review:

::: browser/metro/base/content/downloads.js
@@ +331,5 @@
>          if (this._progressNotificationInfo.size == 0) {
> +          if (this._downloadCount > 1 || !runAfterDownload) {
> +            this._showDownloadCompleteNotification(download);
> +          }
> +          this._downloadCount = 0;

I missed this before: we also need to set _downloadCount = 0 in cancelDownload().  Or maybe observing the dl-cancel notification would take care of that.

::: browser/metro/locales/en-US/chrome/browser.properties
@@ +42,5 @@
>  alertDownloadsSize=Download too big
>  alertDownloadsNoSpace=Not enough storage space
>  alertDownloadSave=Do you want to run or save %S (%S) from %S?
>  alertDownloadMultiple=Downloading one file, #2, #3; Downloading #1 files, #2, #3
> +alertMultipleDownloadsComplete=One download has been completed;#1 downloads have been completed

(Remember to adjust/add localization notes for the new/changed strings here, as per comments on the preceding patches.)

Comment 32

[Matt Brubeck (:mbrubeck)]

If any localizers have time to look at the strings in the final versions of these patches, their feedback would be appreciated.  (The comments above already request some string-related changes to the current versions of the patches, but I'm sure I may have missed some things.  Another upcoming patch to remove old UI should also remove some now-unused strings.)

Comment 33

[Matt Brubeck (:mbrubeck)]

See bug 894949 for some important info regarding PluralForm and LOCALIZATION NOTE comments.

Comment 34

[Vlado Valastiak [:wladow]]

A superquick review of all three patches. I didn't have a free time to go through all the previous comments so I might be duplicating some of them. 

>diff --git a/browser/metro/locales/en-US/chrome/browser.properties b/browser/metro/locales/en-US/chrome/browser.properties
>--- a/browser/metro/locales/en-US/chrome/browser.properties
>+++ b/browser/metro/locales/en-US/chrome/browser.properties

> alertDownloads=Downloads
>-alertDownloadsStart=Downloading: %S
>-alertDownloadsDone=%S has finished downloading
>+alertDownloadsStart=Downloading %S, %S, %S
>+alertDownloadsDone=%S has been downloaded
This needs the identifier to be changed for both strings.
Also proper localization note needs to be added explaining what all the %S parameters represent.
Also please use ordered arguments in alertDownloadsStart

> alertDownloadsNoSpace=Not enough storage space
> alertDownloadSave=Do you want to run or save %S (%S) from %S?
>+alertDownloadMultiple=Downloading one file, #2, #3; Downloading #1 files, #2, #3
Please add a loc note

> alertDownloadsNoSpace=Not enough storage space
>+alertDownloadSave=Do you want to run or save %S (%S) from %S?
Please add a loc note and use ordered arguments.

> downloadCancel=Cancel
>-downloadShowInFiles=Show in Files
>+downloadsShowInFiles=Show in Files;Show them in the Files app
Not sure if we need plural form here, maybe a rewording might help to avoid it?

> alertDownloadMultiple=Downloading one file, #2, #3; Downloading #1 files, #2, #3
>+alertMultipleDownloadsComplete=One download has been completed;#1 downloads have been completed
Please add a loc note here

Comment 35

[Francesco Lodolo [:flod]]

@wladow
It's not really necessary now to change the IDs, being Metro hidden to localizers. It will be mandatory once it's re-enabled and start appearing on the l10n dashboard.

Comment 36

[Francesco Lodolo [:flod]]

Looking at the patch, as a general rule: when multiple variables are present, use ordered arguments and add a localization note explaining each parameter.

downloadsShowInFiles=Show in Files;Show them in the Files app
I think that the plural form would be useful, not sure about the two completely different forms though (e.g. "Show item in Files; Show items in Files").

Comment 37

[Vlado Valastiak [:wladow]]

(In reply to Francesco Lodolo [:flod] from comment #35)
> @wladow
> It's not really necessary now to change the IDs, being Metro hidden to
> localizers. It will be mandatory once it's re-enabled and start appearing on
> the l10n dashboard.

It's never a good practise to introduce exceptions to otherwise straight-forward process. And if the strings are not exposed to the localizers via L10n dashbaord it really doesn't mean the teams are not working on them ;)

Comment 38

[Matt Brubeck (:mbrubeck)]

(In reply to Vlado Valastiak [:wladow] @ Mozilla.sk from comment #37)
> It's never a good practise to introduce exceptions to otherwise
> straight-forward process. And if the strings are not exposed to the
> localizers via L10n dashbaord it really doesn't mean the teams are not
> working on them ;)

I agree -- we'll stick to the usual process of changing string IDs when new translations are needed.  Then we will not develop any bad habits or unforseen consequences.

Comment 39

[Francesco Lodolo [:flod]]

(In reply to Vlado Valastiak [:wladow] @ Mozilla.sk from comment #37)
> It's never a good practise to introduce exceptions to otherwise
> straight-forward process. 

I wouldn't really call it an exception. Metro's localization was dropped because strings weren't stable enough for localization. If someone is following and localizing Metro right now (that's good, I honestly thought I was alone), I think he must be aware that this could happen (and it's already happened, for the record).

But if developers are willing to follow that rule, I'll be glad to open bugs when I see that happen :-)

Comment 40

[Marina Samuel [:emtwo]]

Attachment: Part 4: v1: Removing DownloadsView and Related Code

Note: I removed anything that involved DownloadsView in the existing downloads tests and replaced them with a "TODO" note to do something similar related to the new UI.

Bug 895053 was filed for further testing of downloads.

Comment 41

[Matt Brubeck (:mbrubeck)]

Comment on attachment 777305 [details] [diff] [review]
Part 4: v1: Removing DownloadsView and Related Code

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

Thanks!

::: browser/metro/base/content/downloads.js
@@ +398,5 @@
>    onDownloadStateChange: function dPL_onDownloadStateChange(aState, aDownload) {
>      let state = aDownload.state;
>      switch (state) {
>        case Ci.nsIDownloadManager.DOWNLOAD_QUEUED:
>        case Ci.nsIDownloadManager.DOWNLOAD_BLOCKED_POLICY:

You can remove this whole switch statement; it's currently unused.

DownloadProgressListener overlaps a lot with the dl-* notifications that the Downloads object observes; they're generated based on the same onDownloadStateChange calls:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/nsDownloadManager.cpp#2761

Maybe we should just use this listener to detect state changes, instead of having both the listener and the observers?  I guess it doesn't make a big difference, but feel free to make that change if it makes your life easier.

::: browser/metro/base/tests/mochitest/browser_downloads.js
@@ +253,2 @@
>  
>        for (let i = 0; i < downloadsList.children.length; i++) {

I think this will fail because downloadsList is no longer defined.  If so, you'll need to delete or comment out or fix this before landing this patch.

Comment 42

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

Comment on attachment 777305 [details] [diff] [review]
Part 4: v1: Removing DownloadsView and Related Code

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

I'm picking on you unfairly here. Our test coverage is patchy and not nearly as well documented as I'm asking you for. But, seeing as we're here and talking about it, this would be my personal preference

::: browser/metro/base/tests/mochitest/browser_downloads.js
@@ +246,5 @@
>        // Populate the downloads database with the data required by this test.
>        // we're going to add stuff to the downloads db.
>        yield spawn( gen_addDownloadRows( DownloadData ) );
>  
> +      // TODO: Check that Downloads._progressNotificationInfo and Downloads._downloadCount

can you add a todo("Check that Downloads._progressNotificationInfo and Downloads._downloadCount have the current length"); 

...for this and the other // TODOs below. mochitest provides the todo function as a way to flag known test coverage gaps. todos()s show up in the test run results and are less likely to slip out of sight

Comment 43

[Axel Hecht [:Pike]]

(In reply to Francesco Lodolo [:flod] from comment #36)
> Looking at the patch, as a general rule: when multiple variables are
> present, use ordered arguments and add a localization note explaining each
> parameter.
> 
> downloadsShowInFiles=Show in Files;Show them in the Files app
> I think that the plural form would be useful, not sure about the two
> completely different forms though (e.g. "Show item in Files; Show items in
> Files").

Please don't use plural logic in cases without numbers. Oh, right, did I look at something flod commented on earlier today? Ooops, my bad.

If you don't show the number, all locales for which the singular form is used for 11, 21, etc go wrong.

I'd suggest to do two variants of the string, "_one" and "_not_one", and take it from there.

Comment 44

[Marina Samuel [:emtwo]]

Pushed to Inbound:

Part 1: https://hg.mozilla.org/integration/mozilla-inbound/rev/c23d6553611f
Part 2: https://hg.mozilla.org/integration/mozilla-inbound/rev/84acacfdffec
Part 3: https://hg.mozilla.org/integration/mozilla-inbound/rev/759a92c5b66f
Part 4: https://hg.mozilla.org/integration/mozilla-inbound/rev/83b8090abd16

Comment 45

[Marina Samuel [:emtwo]]

Wasn't using todo() properly. Fixed here:

https://hg.mozilla.org/integration/mozilla-inbound/rev/ac8a15bd0549

Comment 46

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c23d6553611f
https://hg.mozilla.org/mozilla-central/rev/84acacfdffec
https://hg.mozilla.org/mozilla-central/rev/759a92c5b66f
https://hg.mozilla.org/mozilla-central/rev/83b8090abd16
https://hg.mozilla.org/mozilla-central/rev/ac8a15bd0549