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

RESOLVED FIXED in Firefox 25

Status

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: emtwo, Assigned: emtwo)

Tracking

unspecified
Firefox 25
x86_64
Windows 8
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(8 attachments, 8 obsolete attachments)

2.11 MB, image/png
Details
2.11 MB, image/png
Details
2.11 MB, image/png
Details
2.11 MB, image/png
Details
22.23 KB, patch
mbrubeck
: review+
Details | Diff | Splinter Review
11.69 KB, patch
mbrubeck
: review+
Details | Diff | Splinter Review
7.52 KB, patch
mbrubeck
: review+
Details | Diff | Splinter Review
39.87 KB, patch
mbrubeck
: review+
sfoster
: feedback+
Details | Diff | Splinter Review
Assignee

Description

6 years ago
No description provided.
Assignee

Updated

6 years ago
Blocks: 831942
Assignee

Updated

6 years ago
QA Contact: msamuel
Assignee

Comment 1

6 years ago
Posted patch WIP: Download Request Infobar (obsolete) — Splinter Review
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
Attachment #763867 - Flags: feedback?(ally)
Assignee

Updated

6 years ago
Assignee: nobody → msamuel
QA Contact: msamuel
Assignee

Comment 2

6 years ago
Posted image Screenshot of WIP (obsolete) —
Assignee

Comment 3

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

Comment 4

6 years ago
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 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. :)
Attachment #763867 - Flags: feedback?(ally) → feedback+
Assignee

Updated

6 years ago
Summary: Initiate a Download Request Using an InfoBar → Initiate a download request, keep track of its progress, and show downloads completing using an Infobar
Assignee

Comment 6

6 years ago
Very minor changes, I think I just replaced "var" with "let". Will address other comments in a later comment.
Attachment #763867 - Attachment is obsolete: true
Assignee

Comment 8

6 years ago
Attachment #763870 - Attachment is obsolete: true
Looking great, Marina!
Assignee

Comment 13

6 years ago
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 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?
Assignee

Comment 15

6 years ago
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!
(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.
No longer blocks: 831942
Blocks: 886942
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
Attachment #766821 - Flags: feedback?(ally) → feedback+
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
Attachment #766821 - Flags: feedback?(fyan) → feedback+
Assignee

Comment 19

6 years ago
Using the latest pull + clearer patch name
Attachment #766818 - Attachment is obsolete: true
Assignee

Comment 20

6 years ago
* Using the latest pull + clearer patch name
* Addresses comment 17 and comment 18
Attachment #766821 - Attachment is obsolete: true
Assignee

Updated

6 years ago
Attachment #772686 - Attachment is patch: true
Assignee

Updated

6 years ago
Depends on: 883953, 883956
No longer blocks: 886942
Blocks: 893066
Blocks: 886942
No longer blocks: 893066
Assignee

Comment 21

6 years ago
Had positive feedback on this one before. Minor changes since then. Just updating and aiming to ship this bug soon.
Attachment #772683 - Attachment is obsolete: true
Attachment #776412 - Flags: review?(sfoster)
Attachment #776412 - Flags: review?(mbrubeck)
Attachment #776412 - Flags: review?(ally)
Assignee

Comment 22

6 years ago
Also had positive feedback on this one and minor changes afterwards.
Attachment #772686 - Attachment is obsolete: true
Attachment #776416 - Flags: review?(sfoster)
Attachment #776416 - Flags: review?(mbrubeck)
Attachment #776416 - Flags: review?(ally)
Assignee

Comment 23

6 years ago
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?
Attachment #776417 - Flags: review?(sfoster)
Attachment #776417 - Flags: review?(mbrubeck)
Attachment #776417 - Flags: review?(ally)
Assignee

Comment 24

6 years ago
Oops that was not the right file!
Attachment #776412 - Attachment is obsolete: true
Attachment #776412 - Flags: review?(sfoster)
Attachment #776412 - Flags: review?(mbrubeck)
Attachment #776412 - Flags: review?(ally)
Assignee

Updated

6 years ago
Attachment #776423 - Flags: review?(sfoster)
Attachment #776423 - Flags: review?(mbrubeck)
Attachment #776423 - Flags: review?(ally)
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.
Attachment #776423 - Flags: review?(mbrubeck) → review-
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!
Attachment #776423 - Flags: review- → review+
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.
Attachment #776416 - Flags: review?(mbrubeck) → review+
Assignee

Updated

6 years ago
Attachment #776423 - Flags: review?(sfoster)
Attachment #776423 - Flags: review?(ally)
Assignee

Updated

6 years ago
Attachment #776416 - Flags: review?(sfoster)
Attachment #776416 - Flags: review?(ally)
Assignee

Updated

6 years ago
Attachment #776417 - Flags: review?(sfoster)
Attachment #776417 - Flags: review?(ally)
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);
  }
Attachment #776417 - Flags: review?(mbrubeck) → review-
Assignee

Comment 30

6 years ago
The above patch addresses issues in comment 28.
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.)
Attachment #776648 - Flags: review?(mbrubeck) → review+
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.)
See bug 894949 for some important info regarding PluralForm and LOCALIZATION NOTE comments.
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
@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.
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").
(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 ;)
(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.
(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 :-)
Assignee

Comment 40

6 years ago
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.
Attachment #777305 - Flags: review?(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.
Attachment #777305 - Flags: review?(mbrubeck) → review+
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
Attachment #777305 - Flags: feedback+
(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.
Assignee

Comment 45

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/ac8a15bd0549
Blocks: 896156
No longer depends on: 883956
You need to log in before you can comment on or make changes to this bug.