Closed Bug 893763 Opened 6 years ago Closed 6 years ago

Work - Handle network issues and crashes gracefully when downloading

Categories

(Firefox for Metro Graveyard :: Downloads, defect)

x86_64
Windows 8
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 26

People

(Reporter: emtwo, Assigned: sfoster)

References

Details

(Whiteboard: [preview] feature=work)

Attachments

(3 files, 2 obsolete files)

No description provided.
Blocks: 893066
Whiteboard: [preview]
I'll take this. I'm assuming parity with desktop's behavior wrt. disconnections and crashes is the goal and this bug is mostly about getting the UI in the right state. So, p=3
Assignee: nobody → sfoster
Blocks: metrov1it13
No longer blocks: metrov1it13
Summary: Handle network issues and crashes gracefully when downloading → Work - Handle network issues and crashes gracefully when downloading
Whiteboard: [preview] → [preview] feature=work
The profile changes necessary to allow auto-discovery and install of extensions in your profile.
As a prerequisite for this ticket (downloads handling after a crash), this zip is a version of the crashme extension with Metro hacked in. It adds the Metro appid to the install.rdf, and a Settings -> Crash Now! entry which should crash the browser. 

If this looks useful, I can iterate to get the changes into the extension for real on http://code.google.com/p/crashme/ (no pull requests on google code - sadface)
Attachment #793768 - Flags: feedback?(ted)
Patch with just the changes to install.rdf and bootstrap.js.
Attachment #793768 - Attachment is obsolete: true
Attachment #793768 - Flags: feedback?(ted)
Attachment #793869 - Flags: feedback?(ted)
Comment on attachment 793869 [details] [diff] [review]
Modified crashme extension to add Metro support

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

Looks sane. If this works for you then that's fine with me. I'll land it in the Google Code repo and put up a repacked version of the extension on my people account. (Sorry about Google Code, but it gets so few commits that it's less hassle than moving it to github.)
Attachment #793869 - Flags: feedback?(ted) → feedback+
Ok so I tested out the .xpi and it looks good and works well. I'll be updating our wiki with instructions on how to install and use it, so let me know when its up on google code.
Depends on: 895053
Attached patch WIP: notify of resumed downloads (obsolete) — Splinter Review
This is where I'm at. I've refactored a little and I've got any resuming downloads being correctly handled at startup. But it looks like the notifications are being hidden immediately at startup? As far as I can tell from the logging, all the right things are happenning. And sometimes you get a brief flash of what might be the notification bar disappearing as the rest of the UI displays. Ideas?
Attachment #801058 - Flags: feedback?(mbrubeck)
Am wondering if this is related to bug 906772. If notifications are re-triggered at startup while the navbar is visible, they may be hiding behind the navbar. Though I still haven't looked at this patch, this is just an idea.
Yeah I wondered the same. I've got logging in CAO now and I see that notifications are *not* hidden at startup when there's a resumed download, but allNotifications.length == 0. So I'm still missing something when creating those notifications. It so almost works :)
This adds a step to check for active downloads at Downloads.init and notify the user just like we do for "new" downloads. Did a little refactoring to this end. 

Now, if you quit while one or more downloads are in progress, when you next startup you'll get a notification - and an opportunity to cancel resumed downloads. The same happens after a crash. For network disconnects, the download is paused. It will resume when network connectivity is restored. If you quit while disconnected, we'll try to resume when you next startup. 

Note that we don't currently show the download progress button on about:start. So you don't see the progress when you start up and get those notifications. I propose to fix that (in another bug)
Attachment #801058 - Attachment is obsolete: true
Attachment #801058 - Flags: feedback?(mbrubeck)
Attachment #801889 - Flags: review?(mbrubeck)
Comment on attachment 801889 [details] [diff] [review]
Notify of resuming downloads at startup

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

::: browser/metro/base/content/downloads.js
@@ +152,5 @@
>    },
>  
>    // Cancels all downloads.
>    cancelDownloads: function dh_cancelDownloads() {
> +    for (let [guid,info] of this._progressNotificationInfo) {

Nit: Add a space after "," please.  (Rationale: guid,info looks too much like guid.info.)

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

should that be "||" instead of "&&"?
Attachment #801889 - Flags: review?(mbrubeck) → review+
Ah, that should have been !isNan() (is a number and is greater than 0). Fixed that and the other nits and landed on fx-team: 
https://hg.mozilla.org/integration/fx-team/rev/23e8350ce32c
https://hg.mozilla.org/mozilla-central/rev/23e8350ce32c
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
No longer depends on: 895053
You need to log in before you can comment on or make changes to this bug.