Make the download button in the appbar toggle the downloads infobar

RESOLVED FIXED in Firefox 26

Status

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: emtwo, Assigned: emtwo)

Tracking

unspecified
Firefox 26
x86_64
Windows 8
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [preview])

Attachments

(1 attachment, 3 obsolete attachments)

Assignee

Description

6 years ago
No description provided.
Assignee

Updated

6 years ago
Blocks: 831942
Assignee

Updated

6 years ago
Assignee: nobody → msamuel
Assignee

Updated

6 years ago
Summary: Handle Download Complete in the InfoBar → Make the download button in the appbar toggle the downloads infobar
Assignee

Comment 1

6 years ago
This patch needs to be placed on top of the patches in bug 883951.
Attachment #772230 - Flags: feedback?(sfoster)
Attachment #772230 - Flags: feedback?(ally)
Assignee

Updated

6 years ago
Attachment #772230 - Flags: feedback?(sfoster)
Attachment #772230 - Flags: feedback?(ally)
Assignee

Comment 2

6 years ago
Updated with the latest pull so that it applies cleanly on top of patches from bug 883951.

Note: while the infobar is toggled with this patch, this bug does not address UI/behaviour changes that affect how/where the bar is displayed (i.e. it is hidden when the context menu appears). These will be addressed as part of bug 886942.
Attachment #772230 - Attachment is obsolete: true
Attachment #772693 - Flags: feedback?(sfoster)
Comment on attachment 772693 [details] [diff] [review]
V2: Clicking the download button on the appbar toggles the download infobar.

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

Looks good so far, looking forward to see how you test this stuff :)

::: browser/metro/base/content/appbar.js
@@ +108,5 @@
> +          Downloads._showDownloadCompleteNotification(this._xpcomObj);
> +          break;
> +      }
> +      this._xpcomObj = null;
> +      this._currNotification = null;

I have to take your word for it that this logic is correct. You've tested multiple simultaneous downloads and notifications? And failure cases? What happens if you lose internet connection - is the state left correct?

::: browser/metro/base/content/downloads.js
@@ +208,5 @@
>            file.reveal();
>          }
>        }
>      ];
> +    let observerString = "download-complete";

maybe observerTopic = "download-complete" or just topic =

@@ +271,5 @@
>            }
>          }
>        ];
>  
> +      let observerString = "download-progress";

ditto

::: browser/metro/components/HelperAppDialog.js
@@ +97,5 @@
>      downloadSize = this._getDownloadSize(aLauncher.contentLength);
>  
>      let msg = browserBundle.formatStringFromName("alertDownloadSave", [aLauncher.suggestedFileName, downloadSize, aLauncher.source.host], 3);
>  
> +    let observerString = "save-download";

ditto
Attachment #772693 - Flags: feedback?(sfoster) → feedback+
Assignee

Comment 4

6 years ago
In reply to comment 3:

* Made the small nit changes.
* Right now when there are network issues, a download is paused and resumed when the issues are fixed but the UI isn't very clear about what's going on. That will be taken care of in bug 893763.
* Multiple simultaneous downloads and notifications are a part of bug 883951 which still requires a couple of small changes but you're right that for this patch, if there are multiple notifications only the topmost one will be toggled. I'll update this patch to handle that once bug 883951 is taken care of.

tl;dr: Will update/ship bug 883951 before getting back to this one
Attachment #772693 - Attachment is obsolete: true
Edits incorporated and work completed.  Will land as a bundle in a future iteration.
Blocks: 886942
No longer blocks: 831942
Whiteboard: [preview]
Assignee

Comment 6

6 years ago
Apparently there is a much simpler way of doing this! Wish I found it earlier.
Attachment #775633 - Attachment is obsolete: true
Attachment #786388 - Flags: feedback?(sfoster)
Assignee

Updated

6 years ago
Attachment #786388 - Flags: feedback?(sfoster) → review?(sfoster)
Assignee

Updated

6 years ago
Attachment #786388 - Attachment description: toggle-notificationbox → v4: Clicking the download button on the appbar toggles the download infobar
You'll have to step me through this because as your reviewer I feel I should understand this simpler way too! And its not immediately clear to me looking across the patches.
Assignee

Comment 8

6 years ago
Ah sorry for being unclear. Before I was manually keeping track of the notifications using observers. I would remove them to 'hide' them and reappend them to 'show' them. However, I noticed there is a notificationbox attribute, "notificationsHidden", which can simply be set to true or false to show or hide all notifications at once.
Comment on attachment 786388 [details] [diff] [review]
v4: Clicking the download button on the appbar toggles the download infobar

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

I look forward to seeing bug 895053 land, it niggles at me r+ing a series of patches without automated tests. But yes, this looks good and works well.
Attachment #786388 - Flags: review?(sfoster) → review+
Assignee

Updated

6 years ago
Whiteboard: [preview] → [preview][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/7edbd10a7d03
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [preview][fixed-in-fx-team] → [preview]
Target Milestone: --- → Firefox 26
You need to log in before you can comment on or make changes to this bug.