This bug is about the part of the work defined by bug 1062966 related to adding a confirmation dialog when the user chooses to unblock a download. This should be based on the mockups in bug 1053890 and their evolution, as well as the strings in bug 1063105.

The point estimate on this bug probably depends on how much the dialog should be similar to the mockup, I believe 5 can be fine for a custom dialog with tests.

The strings used by DownloadUIHelper.jsm are added in toolkit mozapps. The new strings which will be used by this bug are in browser. Will this functionality be used across different apps and should reside in toolkit? (this means that I'll have to move the strings there).

(In reply to Alex Bardas :alexbardas from comment #2) > Will this functionality be used across different apps and should reside in > toolkit? (this means that I'll have to move the strings there). No, this is a front-end functionality only. This dialog cannot actually be implemented before the new panel item state in bug 1068656, as it will be linked to the unblock functionality there.

I've uploaded a screenshot with how the dialog looks in my patch right now (wip) and how it should look according to the design. There are 2 custom buttons in the design. Dao, what do you think about how those buttons should be implemented and about the colors? What do you say if we go with native feel & look first time?

Sevaan, I've uploaded an image with my wip and the actual design. Do you think we can keep the current alert icon (with the black ! in it) that we're also using in other places? Please also confirm if the colors that will be used for the buttons are those defined here http://people.mozilla.org/~jgruen/chameleon/old/#nav5 : blue: #0095DD; red: #D74345;

I followed Matt suggestion to use a window watcher and add a class at runtime (which shows an alert instead of a question icon). I felt the need to refactor the tests to be able to add more tests for the dialog.

(In reply to Alex Bardas :alexbardas from comment #4) > Created attachment 8500734 [details] > implemented_vs_design.png > > I've uploaded a screenshot with how the dialog looks in my patch right now > (wip) and how it should look according to the design. > > There are 2 custom buttons in the design. > > Dao, what do you think about how those buttons should be implemented and > about the colors? What do you say if we go with native feel & look first > time? Sounds good to me. From what I can tell this is a standard OS X prompt, so it's unclear to me why we'd use non-standard buttons in the first place.

(In reply to Alex Bardas :alexbardas from comment #5) > Sevaan, I've uploaded an image with my wip and the actual design. > > Do you think we can keep the current alert icon (with the black ! in it) > that we're also using in other places? > > Please also confirm if the colors that will be used for the buttons are > those defined here http://people.mozilla.org/~jgruen/chameleon/old/#nav5: > > blue: #0095DD; > red: #D74345; Thanks Alex, Yes, the icon with the black is fine, and those are the correct colour codes.

Comment on attachment 8500864 [details] [diff] [review] Add confirmation dialog to unblock downloads + refactor tests from browser/...downloads v1 Review of attachment 8500864 [details] [diff] [review]: ----------------------------------------------------------------- I didn't look thoroughly at the whole patch but I have some comments. ::: browser/components/downloads/DownloadsCommon.jsm @@ +561,5 @@ > + "chrome://global/content/commonDialog.xul") { > + Services.ww.unregisterNotification(onOpen); > + let dialog = subj.document.getElementById("commonDialog"); > + if (dialog) { > + dialog.classList.add("alert-dialog"); Add a comment about what this is for. e.g. "Change the dialog to use a warning icon for the confirmation" ::: toolkit/themes/linux/global/global.css @@ +59,5 @@ > .message-icon { > list-style-image: url("moz-icon://stock/gtk-dialog-info?size=dialog"); > } > > +.alert-dialog .question-icon, .alert-icon { add a new line after the comma for all 3 CSS files. We put one rule per line usually. ::: toolkit/themes/osx/global/global.css @@ +77,5 @@ > .message-icon { > list-style-image: url("chrome://global/skin/icons/information-64.png"); > } > > +.alert-dialog .question-icon, .alert-icon { This new rule is confusing since it's mixing alert and question together. You shouldn't use .question-icon in the rule and should instead use another reference to the icon element e.g. the ID.

I'm uploading a new version with Matt suggestions. I've also talked with Gavin about it and he suggested (as Dao) we shouldn't style the buttons in this bug (the ux should be consistent).

Comment on attachment 8502252 [details] [diff] [review] Add confirmation dialog to unblock downloads + refactor tests from browser/...downloads v2 Review of attachment 8502252 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/downloads/DownloadsCommon.jsm @@ +147,5 @@ > + * Constants with the different types of unblock messages. > + */ > + UNBLOCK_MALWARE: "Malware", > + UNBLOCK_UNWANTED: "PotentiallyUnwanted", > + UNBLOCK_UNCOMMON: "Uncommon", For consistency, these would be BLOCK_VERDICT_MALWARE, BLOCK_VERDICT_POTENTIALLY_UNWANTED, and BLOCK_VERDICT_UNCOMMON. http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/csd.pb.h#1186 (Unfortunately in strings and other places we use Malware instead of Dangerous, so this will not be an exact match.) @@ +531,5 @@ > + * > + * @return True to unblock the file, false to keep the user safe and > + * cancel the operation. > + */ > + confirmUnblockDownload: function DP_confirmUnblockDownload(aType, aOwnerWindow) { This should probably be a Task.async function for forward compatibility (even though the prompter spins a nested event loop for now anyways). @@ +536,5 @@ > + let prompter = DownloadUIHelper.getPrompter(aOwnerWindow)._prompter; > + // Always continue in case we have no prompter implementation > + if (!prompter) { > + return false; > + } You don't need to use the download prompter internals, just use Services.prompt directly. Also the existence check is not necessary, as we always have a prompter in the Desktop front-end. @@ +542,5 @@ > + let title = s.unblockHeader; > + let buttonFlags = (Ci.nsIPrompt.BUTTON_TITLE_IS_STRING * Ci.nsIPrompt.BUTTON_POS_0) + > + (Ci.nsIPrompt.BUTTON_TITLE_IS_STRING * Ci.nsIPrompt.BUTTON_POS_1); > + > + let type = s["unblockType" + aType] ? s["unblockType" + aType] : ""; nit: I'd prefer if you don't concatenate string names ("unblockType" + aType), but use a switch statement to select the right strings. This makes string usage easier to find in the source code when starting from the localization file. @@ +562,5 @@ > + Services.ww.unregisterNotification(onOpen); > + let dialog = subj.document.getElementById("commonDialog"); > + if (dialog) { > + // Change the dialog to use a warning icon. > + dialog.classList.add("alert-dialog"); So, you're not setting the "alert-icon" class on the "info.icon" element directly, because of possible race conditions with the code in CommonDialog.jsm? In any case, we should file a follow-up bug (if one doesn't exist already) to add an API to specify the icon class when invoking a dialog, and remove this code. A new JavaScript-only API seems a good choice here - all of the dialog code is in JavaScript already. ::: browser/components/downloads/test/browser/browser.ini @@ +7,5 @@ > skip-if = os == "linux" # Bug 949434 > [browser_overflow_anchor.js] > skip-if = os == "linux" # Bug 952422 > +[browser_confirm_unblock_download.js] > +skip-if = os == "linux" # Bug 952422 Do you have any reason to think bug 952422 applies to the new test as well? ::: browser/components/downloads/test/browser/browser_basic_functionality.js @@ +10,5 @@ > /** > * Make sure the downloads panel can display items in the right order and > * contains the expected data. > */ > +add_task(function*() { Thanks for the refactoring! Please add a name (test_...) to the test functions. @@ +53,5 @@ > let dataItem = new DownloadsViewItemController(element).dataItem; > is(dataItem.state, DownloadData[i].state, "Download states match up"); > } > + } catch (ex) { > + ok(false, ex); This should be handled by the test infrastructure, no need to catch explicitly. ::: browser/components/downloads/test/browser/browser_confirm_unblock_download.js @@ +6,5 @@ > +// Tests the dialog which allows the user to unblock a downloaded file. > + > +registerCleanupFunction(() => {}); > + > +function addObserver(button) { A more defined function name, and parameter name, would help readability. @@ +12,5 @@ > + Services.ww.registerNotification(function onOpen(subj, topic, data) { > + if (topic == "domwindowopened" && subj instanceof Ci.nsIDOMWindow) { > + // The test listens for the "load" event which guarantees that the alert > + // class has already been added (it is added when "DOMContentLoaded" is) > + // fired. typo: closing parenthesis @@ +35,5 @@ > + }); > +} > + > +add_task(function*() { > + addObserver("cancel"); Missing "yield" (also below) ::: browser/components/downloads/test/browser/browser_first_download_panel.js @@ +11,5 @@ > + > + // Set the preference instead of clearing it afterwards to ensure the > + // right value is used no matter what the default was. This ensures the > + // panel doesn't appear and affect other tests. > + Services.prefs.setBoolPref("browser.download.panel.shown", gOldPrefValue); I think you can register cleanup functions inside test tasks. This way, you can remove the global variable. @@ +29,1 @@ > } catch(ex) { } Hm, is the try-catch still needed? ::: toolkit/components/prompts/content/commonDialog.xul @@ +24,5 @@ > <script type="application/javascript" src="chrome://global/content/globalOverlay.js"/> > + <script type="application/javascript"> > + document.addEventListener("DOMContentLoaded", function() { > + commonDialogOnLoad(); > + }); Do we still need to change the point where the initialization function is invoked?

(In reply to :Paolo Amadini from comment #11) > Comment on attachment 8502252 [details] [diff] [review] > Add confirmation dialog to unblock downloads + refactor tests from > browser/...downloads v2 > > Review of attachment 8502252 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/components/downloads/test/browser/browser.ini > @@ +7,5 @@ > > skip-if = os == "linux" # Bug 949434 > > [browser_overflow_anchor.js] > > skip-if = os == "linux" # Bug 952422 > > +[browser_confirm_unblock_download.js] > > +skip-if = os == "linux" # Bug 952422 > > Do you have any reason to think bug 952422 applies to the new test as well? It's hard to tell here, but I'll investigate it a bit. I added an intermittent orange in another bug by not using the skip-if the other tests had.

(In reply to Alex Bardas :alexbardas from comment #12) > It's hard to tell here, but I'll investigate it a bit. I added an > intermittent orange in another bug by not using the skip-if the other tests > had. Yeah, unfortunately we don't know the cause of the failures, so we can only guess. I would two Linux try builds, one with both this test and an existing disabled test re-enabled, and another with just this test enabled. If they have different failure rates after several retriggers, we can probably keep this test enabled.

(In reply to :Paolo Amadini from comment #11) > Comment on attachment 8502252 [details] [diff] [review] > Add confirmation dialog to unblock downloads + refactor tests from > browser/...downloads v2 > > Review of attachment 8502252 [details] [diff] [review]: > ----------------------------------------------------------------- > > + // Change the dialog to use a warning icon. > > + dialog.classList.add("alert-dialog"); > > So, you're not setting the "alert-icon" class on the "info.icon" element > directly, because of possible race conditions with the code in > CommonDialog.jsm? Yes, this is the reason I'm adding the class on the dialog element and not "info.icon" itself. > > In any case, we should file a follow-up bug (if one doesn't exist already) > to add an API to specify the icon class when invoking a dialog, and remove > this code. A new JavaScript-only API seems a good choice here - all of the > dialog code is in JavaScript already. This would be nice and something that also Gijs suggested. Do you think it's worth adding this js-only dialog in this bug? And also, do you think it's ok to keep the existing xul file (commonDialog.xul) and its functionality and just add flexibility for the arguments with which it is invoked, or a new .xul file?

(In reply to Alex Bardas :alexbardas from comment #14) > Do you think it's worth adding this js-only dialog in this bug? I'm not sure what you mean by this. I'm guessing you mean the API. I think this is fine for now. > And also, do you think it's ok to keep the existing xul file (commonDialog.xul) and its > functionality and just add flexibility for the arguments with which it is > invoked, or a new .xul file? If we're just changing the icon and possibly button, I don't see why we would want a new XUL file.

(In reply to Alex Bardas :alexbardas from comment #14) > Do you think it's worth adding this js-only dialog in this bug? We'd need to track this separately, but if creating the API turns out to be simpler, with less code than the workaround implemented here, it might be worth going that route from the start. I've no idea about which way is simpler, MattN may have a more informed opinion.

I mostly implemented all review suggestions but: - We still need to call commonDialogOnLoad() on "DOMContentLoaded" in commonDialog.xul to avoid race conditions in tests - Didn't add yield for addObserver(...) because that method must be called in a synchronous manner I'll also run the try build with the scenarios from comment 13 .

Try results for the patch: https://tbpl.mozilla.org/?tree=Try&rev=56d8d9ccd317

Comment on attachment 8502761 [details] [diff] [review] Add confirmation dialog to unblock downloads + refactor tests from browser/...downloads v3 Review of attachment 8502761 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Alex Bardas :alexbardas from comment #17) > - We still need to call commonDialogOnLoad() on "DOMContentLoaded" in > commonDialog.xul to avoid race conditions in tests This might not be a problem, but redirecting the review to MattN to get a second opinion and a final pass, since this affects shared dialog code. > - Didn't add yield for addObserver(...) because that method must be > called in a synchronous manner I see, the promise is only resolved after the click is simulated, at which point the function being tested completes anyways. So, after all you probably don't need to "return new Promise" in the helper function. ::: browser/components/downloads/DownloadsCommon.jsm @@ +149,5 @@ > + * Constants with the different types of unblock messages. > + */ > + BLOCK_VERDICT_MALWARE: "unblockTypeMalware", > + BLOCK_VERDICT_POTENTIALLY_UNWANTED: "unblockTypePotentiallyUnwanted", > + BLOCK_VERDICT_UNCOMMON: "unblockTypeUncommon", nit: I'd prefer if you kept the constant values like before (just "Malware" and so on), and used the full string name like s["unblockTypeMalware"] inside the switch below.

Removed the promise from the test because it was unused and renamed the values of the properties from DownloadsCommon.

Comment on attachment 8504902 [details] [diff] [review] Add confirmation dialog to unblock downloads + refactor tests from browser/...downloads v4 Review of attachment 8504902 [details] [diff] [review]: ----------------------------------------------------------------- I think the common dialog changes are fine if Try agrees.

The try run looks good. I had some small changes since I ran it, but they shouldn't affect anything. I mark it as checkin-needed.

Testing this appears to be blocked by bug 1068656.