Closed Bug 905123 Opened 6 years ago Closed 6 years ago

Move prompts from DownloadIntegration to DownloadPrompter

Categories

(Toolkit :: Downloads API, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: Paolo, Assigned: andreshm)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

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".
Assignee: nobody → andres
Status: NEW → ASSIGNED
Attached patch Patch v1 (obsolete) — Splinter Review
Attachment #797025 - Flags: review?(paolo.mozmail)
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.
Attachment #797025 - Flags: review?(paolo.mozmail)
Attached patch bug-905123.patch (obsolete) — Splinter Review
Applied changes as requested.
Attachment #797025 - Attachment is obsolete: true
Attachment #798391 - Flags: review?(paolo.mozmail)
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;
Attachment #798391 - Flags: review?(paolo.mozmail) → review+
Attached patch bug-905123.patchSplinter Review
Applied changes.

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=284ae63e5483
Attachment #798391 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/4880f3205221
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.