896927 - Handle the executable warning prompt

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[:Paolo Amadini]

In the new JavaScript API for downloads, before calling the "launch" method
on a downloaded executable file, we should provide a warning prompt if the
operating system does not include one already.

We might let the caller do this interactive operation when appropriate, with
the actual prompt implemented in the DownloadUIHelper module. The Download
object could provide helper functions to determine if the prompt is required,
and/or the UI helper module could have a method that takes a Download object
as an argument and uses that to determine if the prompt is required.

Comment 1

[:Paolo Amadini]

Attachment: The patch

This moves the executable warning prompt to a common module, both for the
Library window (past downloads) and the Downloads Panel (current downloads).
The warning is more strict in that it assumes the executable file has not been
marked for the operating system level prompt. You'll notice that the check
for the scanning preference is missing, since we don't scan at present.

The string loading code has also been moved from the Firefox-specific module to
the DownloadUIHelper module. The "strings" getter is still available in the
Firefox module so that callers don't have to import the other module as well.

The DownloadUIHelper module is independent from the rest of the Downloads API,
so we can use it in the Library window regardless of the activation preference.
We'll land this bug vary near to the preference switch in any case.

Comment 2

[Neil Deakin]

Comment on attachment 790169 [details] [diff] [review]
The patch

-      let showAlert = true;
+      // We get a prompter for the provided window here, even though anchoring
+      // to the most recently active window should work as well.
+      promiseShouldLaunch = DownloadUIHelper.getPrompterForWindow(aOwnerWindow)
+                                          .confirmLaunchExecutable(aFile.path);

Fix indentation here.


 /**
+ * Returns an object whose keys are the string names from the downloads string
+ * bundle, and whose values are either the translated strings or functions
+ * returning formatted strings.
+ */
+XPCOMUtils.defineLazyGetter(DownloadsCommon, "strings", function () {
+  return DownloadUIHelper.strings;
+});
+

Is this wrapper here because DownloadUIHelper shouldn't be used externally?


 this.DownloadUIHelper = {
+  /**
+   * Returns an object that can be used to display message boxes in response to
+   * global notifications that are not associated with any window.
+   *
+   * @return A DownloadPrompter object.
+   */
+  getPrompter: function ()
+  {
+    return new DownloadPrompter(null);
+  },


Not sure there's a need for both getPrompter and getPrompterForWindow when the only difference
is a null window, which could just be the default argument.


Since you always just call confirmLaunchExecutable right after getting the prompter, why is the prompter
object even necessary? Why not just have DownloadUIHelper.confirmLaunchExecutable?

+/**
+ * Allows displaying anchored to a window.
+ *

It seems like there's a word/phrase missing after 'displaying'.


+      if (shouldLaunch) {
+        Services.prefs.setBoolPref(kPrefAlertOnEXEOpen, !checkState.value);
+      }

This sets the preference even when the user didn't change the checkbox, right? Do we normally write 'remember this decision' type code this way?

Comment 3

[:Paolo Amadini]

(In reply to Neil Deakin from comment #2)
> +XPCOMUtils.defineLazyGetter(DownloadsCommon, "strings", function () {
> 
> Is this wrapper here because DownloadUIHelper shouldn't be used externally?

It will be used in the future, but for now callers only import DownloadsCommon,
so we keep compatibility and don't force them to import another module just to
get the strings. We'll do a lot of refactoring here when we'll remove the
support for the old nsIDownloadManager anyways.

> Since you always just call confirmLaunchExecutable right after getting the
> prompter, why is the prompter
> object even necessary? Why not just have
> DownloadUIHelper.confirmLaunchExecutable?

We'll add more prompts there in bug 905123. Also, having the common aWindow
parameter abstracted makes it easier to have more state associated with the
prompter, or cleaner indirection (getPrompterForDocument, and so on).

> +      if (shouldLaunch) {
> +        Services.prefs.setBoolPref(kPrefAlertOnEXEOpen, !checkState.value);
> +      }
> 
> This sets the preference even when the user didn't change the checkbox,
> right? Do we normally write 'remember this decision' type code this way?

I can do either way. The advantage of this one is that we set the preference
to false and don't get the exception the next time we read it (current mode).
The disadvantage is that we always add a new preference even if unneeded.
Let me know what you think.

Comment 4

[:Paolo Amadini]

Attachment: Updated patch

Comment 5

[Neil Deakin]

Comment on attachment 790470 [details] [diff] [review]
Updated patch

> I can do either way. The advantage of this one is that we set the preference
> to false and don't get the exception the next time we read it (current mode).
> The disadvantage is that we always add a new preference even if unneeded.
> Let me know what you think.

In this case I don't think it matters too much, since the default case is to show the dialog.

Comment 6

[:Paolo Amadini]

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

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Backed out for WinXP xpcshell failures.
https://hg.mozilla.org/integration/fx-team/rev/4b4f81ad9c60

https://tbpl.mozilla.org/php/getParsedLog.php?id=26654952&tree=Fx-Team

Comment 8

[:Paolo Amadini]

Attachment: Use the right string bundle

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #7)
> Backed out for WinXP xpcshell failures.

My bad, this used the wrong string bundle. Removed the union and re-landed:

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

Comment 9

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/ce91946a60eb