883962 - Make the download button in the appbar toggle the downloads infobar

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: Downloads, defect)

Comment 1

[Marina Samuel [:emtwo]]

Attachment: Clicking the download button on the appbar toggles the download infobar.

This patch needs to be placed on top of the patches in bug 883951.

Comment 2

[Marina Samuel [:emtwo]]

Attachment: V2: Clicking the download button on the appbar toggles the download infobar.

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.

Comment 3

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

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

Comment 4

[Marina Samuel [:emtwo]]

Attachment: V3: Clicking the download button on the appbar toggles the download infobar.

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

Comment 5

[Marco Mucci [:MarcoM]]

Edits incorporated and work completed.  Will land as a bundle in a future iteration.

Comment 6

[Marina Samuel [:emtwo]]

Attachment: v4: Clicking the download button on the appbar toggles the download infobar

Apparently there is a much simpler way of doing this! Wish I found it earlier.

Comment 7

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

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.

Comment 8

[Marina Samuel [:emtwo]]

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 9

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

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.

Comment 10

[Marina Samuel [:emtwo]]

https://hg.mozilla.org/integration/fx-team/rev/7edbd10a7d03

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/7edbd10a7d03