Closed Bug 896927 Opened 8 years ago Closed 8 years ago

Handle the executable warning prompt

Categories

(Toolkit :: Downloads API, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: Paolo, Assigned: Paolo)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Blocks: 905123
Attached patch The patch (obsolete) — Splinter Review
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.
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Attachment #790169 - Flags: review?(enndeakin)
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?
(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.
Attached patch Updated patch (obsolete) — Splinter Review
Attachment #790169 - Attachment is obsolete: true
Attachment #790169 - Flags: review?(enndeakin)
Attachment #790470 - Flags: review?(enndeakin)
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.
Attachment #790470 - Flags: review?(enndeakin) → review+
(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
Attachment #790470 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/ce91946a60eb
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Depends on: 907047
Depends on: 908048
You need to log in before you can comment on or make changes to this bug.