905123 - Move prompts from DownloadIntegration to DownloadPrompter

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[:Paolo Amadini]

The new DownloadPrompter object in the DownloadUIHelper module should be used
to display message boxes in response to notifications. This will replace the
use of gStringBundle in DownloadIntegration with "DownloadUIHelper.strings".

Comment 1

[Andres Hernandez [:andreshm]]

Attachment: Patch v1

Comment 2

[:Paolo Amadini]

Comment on attachment 797025 [details] [diff] [review]
Patch v1

The function in DownloadPrompter should not accept the strings to display as
arguments. Instead, it should just get the number of downloads and an argument
with the prompt type (using a constant defined on the DownloadPrompter
prototype), and return a boolean. The available prompt types should be
DownloadPrompter.ON_QUIT, DownloadPrompter.ON_OFFLINE,
DownloadPrompter.ON_LEAVE_PRIVATE_BROWSING.

Comment 3

[Andres Hernandez [:andreshm]]

Attachment: bug-905123.patch

Applied changes as requested.

Comment 4

[:Paolo Amadini]

Comment on attachment 798391 [details] [diff] [review]
bug-905123.patch

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

Looks good, with the changes below. Thanks!

::: toolkit/components/jsdownloads/src/DownloadUIHelper.jsm
@@ +177,5 @@
> +   *        The current downloads count.
> +   * @param aPromptType
> +   *        The type of prompt notification depending on the observer.
> +   *
> +   * @return boolean with the result of the confirm prompt.

"@return True to cancel the downloads and continue, false to abort the operation."

We should do this because it matches the expectations based on the method's name. If I understand correctly, this is inverted with regard to "aCancel.data", that prevents continuing instead.

@@ +199,5 @@
> +      case this.ON_QUIT:
> +        title = s.quitCancelDownloadsAlertTitle;
> +#ifndef XP_MACOSX
> +        message = aDownloadsCount > 1 ? s.quitCancelDownloadsAlertMsgMultiple(aDownloadsCount)
> +                                      : s.quitCancelDownloadsAlertMsg;

May indent like this:

message = aDownloadsCount > 1
          ? s.quitCancelDownloadsAlertMsgMultiple(aDownloadsCount)
          : s.quitCancelDownloadsAlertMsg;

Comment 5

[Andres Hernandez [:andreshm]]

Attachment: bug-905123.patch

Applied changes.

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=284ae63e5483

Comment 6

[Andres Hernandez [:andreshm]]

Try is good https://tbpl.mozilla.org/?tree=Try&rev=284ae63e5483

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4880f3205221

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4880f3205221