Need to rethink and refactor tests/functional/testPrivateBrowsing/testDownloadManagerClosed.js to handle the new PB per-window functionality. See bug 801232 for more info on the implementation.

Lets get this test skipped as soon as possible.

added skip patch

Comment on attachment 693943 [details] [diff] [review] skipPatch v1.0 Review of attachment 693943 [details] [diff] [review]: ----------------------------------------------------------------- This patch doesn't apply anymore. Also there is one more thing you should obey. See my inline comment. ::: tests/functional/testPrivateBrowsing/manifest.ini @@ +1,5 @@ > [testAboutCache.js] > [testAboutPrivateBrowsing.js] > [testDisabledPermissions.js] > [testDownloadManagerClosed.js] > +disabled = Bug 823064 - Refactor testDownloadManagerClosed.js for the new PB per-window mode There is no need to call out the test filename in the skip message. It's already known because it's tied 1-1 to the test.

Comment on attachment 694302 [details] [diff] [review] skipPatch v1.1 Review of attachment 694302 [details] [diff] [review]: ----------------------------------------------------------------- Looks good now and applies cleanly.

I've devised a simple testcase for what this test might do: Our testcase should check that parallel downloads from _Normal Browsing_ vs _Private Browsing_ are kept independend (they are not polluting eachothers streams) ## Testcase 1. Open a Normal window 2. Start downloading some files 3. Open a PB window 4. Start downloading some files (others than on step 2.) 5. Check that the files in the Normal Downloads Indicator (new Arrow Button in the toolbar) are different from the files from the PB Downloads Indicator 6. Check that the Download Manager only shows the regular download files (no PB download files should be in there) -------- We might also need to rename the test. Not sure what would be appropriate: testDownloadManager.js // Not really accurate since we're going to test the Download Indicator more testDownloadIndicatorAndManager.js // Might be or verbose -------- Henrik, please give me your thoughts on this one. Thanks

Comment on attachment 694302 [details] [diff] [review] skipPatch v1.1 Review of attachment 694302 [details] [diff] [review]: ----------------------------------------------------------------- ::: tests/functional/testPrivateBrowsing/testDownloadManagerClosed.js @@ +127,5 @@ > dm.close(); > } > + > +setupModule.__force_skip__ = "Bug 823064 - Refactor testDownloadManagerClosed.js " + > + "for the new PB per-window mode"; the removal of the filename also applies to the skip lines in the test itself and not only to the manifest. Mind doing that quick update?

The download panel hasn't been preffed on by default yet, so how does your proposal look like for the normal download manager? Otherwise it sounds reasonable.

So we might have instances with no new download panel? We don't really have anything to check for in this case, since the old download manager does not show any PB download files. And we do check for that in step 6. tl;dr: step 5. is irrelevant in this case, will not apply step 6. still applies

Comment on attachment 694343 [details] [diff] [review] skipPatch v1.2 Review of attachment 694343 [details] [diff] [review]: ----------------------------------------------------------------- Looks good now.

(In reply to Andrei Eftimie from comment #11) > So we might have instances with no new download panel? > We don't really have anything to check for in this case, since the old > download manager does not show any PB download files. Please talk with Ehsan and Josh about the strategy when m-c gets merge to aurora and the downloads panel is still kept preffed off. In such a case we would need an update to the old download manager window. Not sure if there have been made some thoughts about it yet.

Comment on attachment 694343 [details] [diff] [review] skipPatch v1.2 Review of attachment 694343 [details] [diff] [review]: ----------------------------------------------------------------- > Bug 823064 - Refactor testDownloadManagerClosed.js for the new PB per-window mode. r=hskupin, r=dhunt, r=amatei This commit message does not match what this patch is about. Please do not simply c&p the content from the bug summary. I will update for check-in.

http://hg.mozilla.org/qa/mozmill-tests/rev/f3666a7c5251 (default) Lets get this updated by next week. Putting into the new sprint.

(In reply to Henrik Skupin (:whimboo) [away 12/21 - 01/01] from comment #14) > Comment on attachment 694343 [details] [diff] [review] > skipPatch v1.2 > > Review of attachment 694343 [details] [diff] [review]: > ----------------------------------------------------------------- > > > Bug 823064 - Refactor testDownloadManagerClosed.js for the new PB per-window mode. r=hskupin, r=dhunt, r=amatei > > This commit message does not match what this patch is about. Please do not > simply c&p the content from the bug summary. I will update for check-in. Sorry for that. Don't know why I was under the impression that I needed to have exactly the bug title as a commit message.

(In reply to Henrik Skupin (:whimboo) [away 12/21 - 01/01] from comment #13) > Please talk with Ehsan and Josh about the strategy when m-c gets merge to > aurora and the downloads panel is still kept preffed off. In such a case we > would need an update to the old download manager window. Not sure if there > have been made some thoughts about it yet. I am not sure what information to ask for and how that will help us. We have: 1. The new Download Indicator, which has 2 separate instances (for Normal and for Private) We can test that the downloaded files only appear where they should be (as stated in comment #7) 2. The old Download Manager, under which we can test that the Private downloads do not appear at all. The old DM is always available (with browser.download.useToolkitUI set to both true or false). The difference is which UI element is the default one used (new DI or old DM) If the setting browser.download.useToolkitUI is preffed true, you have no way of seeing PB downloads (at all). It will always open the old DM, and always show only Normal downloads. Thus the (loosely defined) proposition from comment #7 holds good.

The new download panel is preffed off currently in Aurora. So if we update our tests on default to make use of it we will fail on Aurora because it's not available there yet. I think there is a bug on file for our tests to move to the new panel but it most likely will not happen until the feature is ready to get preffed on. Another solution would be to force the new dl panel pref to be enabled so the new download manager will always be available for testing purposes. Anthony, would do you think about? That way we can have those tests ready before we turn the flip.

(In reply to Henrik Skupin (:whimboo) from comment #18) We've decided to not ship the Download Manager changes in Firefox 19; it is currently scheduled for Firefox 20. When Firefox 20 moves to Aurora on Monday, the feature will be pref'd on. A decision will be made before Firefox 20 Beta whether to keep it pref'd off. Given this situation I'd like to have Mozmill able to test both Download Managers. Would it be possible to fork the test to hand both?

No, we should implement two separate tests then. One for the old manager and the other for the panel and library window. Lets keep this bug for the old manager and file a new one for the other.

Remember this is related to the new Private Browsing functionality. Not general testing of the Download Manager and Download Panel or the Library Window. What this test can do is to check whether Normal vs Private downloads don't get mixed up. Therefore I don't recommend splitting this up. We basically need to download some files and check whether the UI is correctly reflecting the downloaded files (normal vs private) in all UI instances. I have a test (almost) ready that does the following: 1. preffes on the new DI 2. downloads a couple of files normally and privately 3. checks that the new panels show the correct downloads in the UI 4. preffes off the new DI 5. checks that the old DM only shows normal downloads (no private ones) If we are going to split them, then Henrik can you please provide some guidance on what we should test?

Please do split those tests. That will make it easier for us to remove the ones for the old DM later when we are going to disable support for it. Otherwise we have mixed-up tests for different features in a single module which can cause us pain to strip off the code for the old DM. Just name the files correctly so we can see which is testing the old and which the new manager.

We have 2 tests: one for the new Download Panel and one for the old Download Manager. Here is a testrun: http://mozmill-crowd.blargon7.com/#/functional/report/23d8fbdd0190d4b0496d6b129ff7b19d

Comment on attachment 700460 [details] [diff] [review] patch v1 Review of attachment 700460 [details] [diff] [review]: ----------------------------------------------------------------- Leaving r? for Henrik's comments. ::: tests/functional/testPrivateBrowsing/testDownloadIndicator.js @@ +43,5 @@ > + // Force Disable the old Download Manager > + prefs.preferences.setPref(PREF_DOWNLOAD_USE_TOOLKIT, false); > + > + // Disable the opening of the Downloads Manager when starting a download > + prefs.preferences.setPref(PREF_DOWNLOAD_SHOW_STARTING, false); Is this necessary is the download manager is disabled? @@ +63,5 @@ > +var testDownloadIndicator = function() { > + > + // Download files of unknown type > + // Normal Browsing > + DOWNLOADS.normal.forEach(function (el, i) { Rename el to aFile, also the index isn't used @@ +68,5 @@ > + downloads.downloadFileOfUnknownType(controller, LOCAL_TEST_FOLDER + el); > + }); > + > + // Private Browsing > + DOWNLOADS.pb.forEach(function (el, i) { Rename el to aFile, also the index isn't used @@ +79,5 @@ > + }, "All downloads have been finished"); > + > + // Open Download Indicator and read the downloaded item list > + controller.click(downloadButton); > + var nodeCollector = new domUtils.nodeCollector(controller.window.document. I believe this should be in downloads.js, so it's abstracted from the test and reusable. @@ +88,5 @@ > + downloadedFiles.push(downloadIndicatorItems[i].attributes["target"].textContent); > + } > + > + // Check that number of normal downloaded files is identical to the original download list > + var intersection = utils.arrayIntersection(downloadedFiles, DOWNLOADS.normal); Why is it necessary to get the intersection of the two arrays? Shouldn't downloadedFiles already equal the length and contents of DOWNLOADS.normal? ::: tests/functional/testPrivateBrowsing/testDownloadManager.js @@ +59,5 @@ > +var testDownloadManager = function() { > + > + // Download files of unknown type > + // Normal Browsing > + DOWNLOADS.normal.forEach(function (el, i) { rename el to aFile @@ +64,5 @@ > + downloads.downloadFileOfUnknownType(controller, LOCAL_TEST_FOLDER + el); > + }); > + > + // Private Browsing > + DOWNLOADS.pb.forEach(function (el, i) { rename el to aFile @@ +78,5 @@ > + dm.open(controller); > + var downloadView = new elementslib.ID(dm.controller.window.document, "downloadView"); > + dm.controller.waitForElement(downloadView); > + > + var nodeCollector = new domUtils.nodeCollector(downloadView.getNode()); Previously we used downloadView.getNode().itemCount to check the number of items, is there a reason we can't check this against DOWNLOADS.normal.length to ensure there are only 2 downloads? @@ +79,5 @@ > + var downloadView = new elementslib.ID(dm.controller.window.document, "downloadView"); > + dm.controller.waitForElement(downloadView); > + > + var nodeCollector = new domUtils.nodeCollector(downloadView.getNode()); > + var dmItems = nodeCollector.queryNodes("richlistitem").nodes; Can we not continue to use "dl" + (i + 1) to check the downloads, rather than this new element locator?

> @@ +79,5 @@ > > + }, "All downloads have been finished"); > > + > > + // Open Download Indicator and read the downloaded item list > > + controller.click(downloadButton); > > + var nodeCollector = new domUtils.nodeCollector(controller.window.document. > > I believe this should be in downloads.js, so it's abstracted from the test and reusable. Good call, refactored in downloads.js Not sure if it should be a method of dm or a standalone function. I've made it a standalone function, since its not really about the Download Manager. But then again, there are more dm methods there that are not really DM stuff. It will probably need a bigger refactoring later one. > @@ +88,5 @@ > > + downloadedFiles.push(downloadIndicatorItems[i].attributes["target"].textContent); > > + } > > + > > + // Check that number of normal downloaded files is identical to the original download list > > + var intersection = utils.arrayIntersection(downloadedFiles, DOWNLOADS.normal); > > Why is it necessary to get the intersection of the two arrays? Shouldn't downloadedFiles already > equal the length and contents of DOWNLOADS.normal? I want to be sure we have the correct downloads. Only checking the lenght of the array of the downloaded files does not ensure we have the correct ones. (If Normal and Private downloads intermix, only checking the length will not be enough, unless we have a different number of downloads). > @@ +78,5 @@ > > + dm.open(controller); > > + var downloadView = new elementslib.ID(dm.controller.window.document, "downloadView"); > > + dm.controller.waitForElement(downloadView); > > + > > + var nodeCollector = new domUtils.nodeCollector(downloadView.getNode()); > > Previously we used downloadView.getNode().itemCount to check the number of items, is there a > reason we can't check this against DOWNLOADS.normal.length to ensure there are only 2 downloads? Same comment as above, length is not sufficient. > @@ +79,5 @@ > > + var downloadView = new elementslib.ID(dm.controller.window.document, "downloadView"); > > + dm.controller.waitForElement(downloadView); > > + > > + var nodeCollector = new domUtils.nodeCollector(downloadView.getNode()); > > + var dmItems = nodeCollector.queryNodes("richlistitem").nodes; > > Can we not continue to use "dl" + (i + 1) to check the downloads, rather than this new element > locator? Same as above.

So with the latest merge the new download panel has not been disabled on Aurora. That means that we should not take care of the older downloads manager anymore and only test the currently enabled by default one.

Correct, the old DM does not appear anywhere anymore. I've removed the old DM test file. But when pressing Cmd+J you get the new Library window (I'm guessing this takes over the role of the old DM). And even weirder is that in a PB window, Cmd+J opens a "contentAreaDownloadsView" in a tab. Confusing :-S

This is in Nightly(21) and Aurora(20) when pressing Cmd(Ctrl)+J in a Private Window. In Normal more the new Library window (with the Downloads "tab" active) is shown.

Comment on attachment 702228 [details] Cmd+J in PB mode This is not a question for me but for Ehsan or Josh. I can't answer that.

Comment on attachment 702226 [details] [diff] [review] patch v3 Clearing request until we know what the right behavior is.

What's the question, exactly? The download library window is opened when invoking the download manager from a public window, and it shows all persistent public downloads. When the download manager action is invoked from a private window, it opens the in-content transient private download list in a tab.

Comment on attachment 702228 [details] Cmd+J in PB mode This is the expected behavior.

Updated to apply cleanly with the latest version of bug 818456

Comment on attachment 702768 [details] [diff] [review] patch v3.1 Review of attachment 702768 [details] [diff] [review]: ----------------------------------------------------------------- This fails for me on Linux, at line 81: "Normal Downloads are correctly shown in the Downloads Indicator Panel - '0' should equal '2'", On OS X works fine though. ::: lib/downloads.js @@ +405,5 @@ > + * > + * @param {MozMillController} controller MozMillController of the browser window > + * @returns {array} list of downloaded file names > + */ > +function getPanelDownloads(controller){ Please use aController (all parameters have to be prefixed with 'a') and add a space between ')' and '{' ::: lib/utils.js @@ +131,5 @@ > + * @param {array} b > + * @return {array} > + * Intersection of a and b > + */ > +function arrayIntersection(a, b){ Space between ')' and '{' @@ +132,5 @@ > + * @return {array} > + * Intersection of a and b > + */ > +function arrayIntersection(a, b){ > + return a.filter(function( n ){ Same as above and function (n) please. Could we have another name for n? Something more specific, aElement maybe? ::: tests/functional/testPrivateBrowsing/testDownloadIndicator.js @@ +3,5 @@ > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +// Include the required modules > +var { assert } = require("../../../lib/assertions"); > +var domUtils = require("../../../lib/dom-utils"); Is this used currently? @@ +38,5 @@ > + dm = new downloads.downloadManager(); > + dm.cleanAll(); > + > + // Force Disable the old Download Manager > + prefs.preferences.setPref(PREF_DOWNLOAD_USE_TOOLKIT, false); Do we still need this if the old Download Manager was disabled? @@ +67,5 @@ > + downloads.downloadFileOfUnknownType(pb.controller, LOCAL_TEST_FOLDER + aFile); > + }); > + > + // Wait until all downloads have been finished > + controller.waitFor(function () { Please use assert.waitFor() here. @@ +86,5 @@ > + // Check that number of pb downloaded files is identical to the original pb download list > + var PBIntersection = utils.arrayIntersection(downloadedPBFiles, DOWNLOADS.pb); > + assert.equal(PBIntersection.length, DOWNLOADS.pb.length, > + "Private Downloads are correctly shown in the Private Downloads Indicator Panel"); > +} Extra line at the end of a file.

Figured out with Andrei why this failed on Linux. I already had the files downloaded in my Downloads folder, from a previous work.

So we should most likely tweak the default download location so it points to a sub folder in %tmp%. That would ensure that we do not clutter the users default folder and don't fail due to existing files in there.

Addressed Andreeas feedback. Also changed the download folder to the OS temp folder + timestamp. Henrik, you suggested on irc to use some other method of generating a unique folder name. I am finding a timestamp to be a simple, reliable and pretty fast solution. If you know a better way, I'll gladly change it if it is better. Thanks

Comment on attachment 705840 [details] [diff] [review] patch v4 Review of attachment 705840 [details] [diff] [review]: ----------------------------------------------------------------- This will no longer aplly cleanly because of changes in downloads lib from bug 830688. There are a few format changes to be made. ::: lib/downloads.js @@ +76,5 @@ > + * > + * @returns {number} Number of active downloads > + */ > + get activePrivateDownloadCount() { > + return this._dms.activePrivateDownloadCount; We have updated all files to use Services.jsm, now it should be Services.downloads in all instances instead of _dms. @@ +369,5 @@ > */ > var downloadFileOfUnknownType = function(controller, url) { > controller.open(url); > > + No need for this extra line. ::: lib/utils.js @@ +131,5 @@ > + * @param {array} aArray2 > + * @return {array} Intersection of aArray1 and aArray2 > + */ > +function arrayIntersection(aArray1, aArray2) { > + return aArray1.filter(function( aElement ) { Here we should have 'function (aElement) {' ::: tests/functional/testPrivateBrowsing/testDownloadIndicator.js @@ +45,5 @@ > +var teardownModule = function(module) { > + // Clean all downloaded files from the system > + dm.cleanAll(); > + > + // Reset the Download Location Don't think we need this comment, is pretty clear from the name itself. @@ +96,5 @@ > + * @return {string} path to the newly created temporary folder > + */ > +function getTempDownloadLocation() { > + var downloadDir = Cc["@mozilla.org/file/directory_service;1"]. > + getService(Ci.nsIProperties). Please use Services.dirsvc.get() instead.

Updated patch to apply cleanly given the latest changes in downloads.js and to use Services in the new way.

Comment on attachment 708099 [details] [diff] [review] patch v5 Review of attachment 708099 [details] [diff] [review]: ----------------------------------------------------------------- It looks good to me know. I will leave r? for Henrik and Dave as well, since it is kind of like a new test (with the new PB feature).

Comment on attachment 708099 [details] [diff] [review] patch v5 Review of attachment 708099 [details] [diff] [review]: ----------------------------------------------------------------- Sorry that it has been taken a bit longer. It looks like that we still have to do a bit of work on this patch. ::: lib/downloads.js @@ +19,4 @@ > var utils = require("utils"); > > const gTimeout = 5000; > +const PREF_DOWNLOAD_LOCATION = "browser.download.dir"; Please try to be close to the pref name and change LOCATION to DIR. @@ +19,5 @@ > var utils = require("utils"); > > const gTimeout = 5000; > +const PREF_DOWNLOAD_LOCATION = "browser.download.dir"; > +const PREF_DOWNLOAD_FOLDERLIST = "browser.download.folderList"; Just curious. What's that? Worth a comment here? @@ +175,2 @@ > */ > + getAllDownloads : function downloadManager_getAllDownloads(type) { All download means for me that we return all whether of which type. So it might be only a wrapper and call getDownloads() for both types. @@ +189,2 @@ > > + function getDownloads(type) { I would change this and make its own member of the class. There will be a lot of cases you want to retrieve specific downloads. nit: aType please and jsdoc usage added. @@ +189,3 @@ > > + function getDownloads(type) { > + if (type === "private") Rather making it a string I think it would be better to use a boolean here, e.g. aIsPrivate. @@ +195,5 @@ > > + var stmt = null; > + > + if (dbConn.schemaVersion < 3) > + return new Array(); When does that happen? I'm sure we can remove that now. Builds which used version 3 are not supported anymore I believe. @@ +213,5 @@ > + preferredAction: stmt.row.preferredAction > + }); > + }; > + stmt.reset(); > + return downloads; Please keep the blank line before the return statement. @@ +318,5 @@ > /** > + * Sets the Download Location > + * @param {string} aPath where to save downloaded files > + */ > + setDownloadLocation : function downloadManager_setDownloadLocation( aPath ) { I would better make it a setter and getter. nit: extra blanks around the parameter. @@ +420,5 @@ > > +/** > + * Get the download filenames from the Download Panel > + * > + * @param {MozMillController} controller MozMillController of the browser window nit: controller -> aController @@ +421,5 @@ > +/** > + * Get the download filenames from the Download Panel > + * > + * @param {MozMillController} controller MozMillController of the browser window > + * @returns {array} list of downloaded file names nit: {[String]} @@ +423,5 @@ > + * > + * @param {MozMillController} controller MozMillController of the browser window > + * @returns {array} list of downloaded file names > + */ > +function getPanelDownloads(aController) { This should be moved into an appropriate ui class instead of having it as a global function in the downloads.js module. @@ +425,5 @@ > + * @returns {array} list of downloaded file names > + */ > +function getPanelDownloads(aController) { > + var button = new elementslib.ID(aController.window.document, "downloads-indicator"); > + aController.click(button); Keep in mind that this will not close the opened popup. @@ +431,5 @@ > + getElementById("downloadsPanel")); > + var downloadIndicatorItems = nodeCollector.queryNodes("richlistitem").nodes; > + var downloadedFiles = []; > + for (var i = 0; i < downloadIndicatorItems.length; i++) { > + downloadedFiles.push(downloadIndicatorItems[i].attributes["target"].textContent); We can use map() here. Which other information are available? ::: tests/functional/testPrivateBrowsing/testDownloadIndicator.js @@ +9,5 @@ > +var downloads = require("../../../lib/downloads"); > +var privateBrowsing = require("../../../lib/ui/private-browsing"); > +var utils = require("../../../lib/utils"); > + > +const DOWNLOAD_LOCATION = getTempDownloadLocation(); This function should be in utils.js. @@ +17,5 @@ > + "unknown_type.fmtd" > + ], > + pb: [ > + "unknown_type_pb.stbf", > + "unknown_type_pb.dets" Why this new file? @@ +20,5 @@ > + "unknown_type_pb.stbf", > + "unknown_type_pb.dets" > + ] > +}; > +const LOCAL_TEST_FOLDER = collector.addHttpResource("../../../data/downloading/"); Please move this up to the top before any other constant and separate by a blank line. @@ +22,5 @@ > + ] > +}; > +const LOCAL_TEST_FOLDER = collector.addHttpResource("../../../data/downloading/"); > + > +var setupModule = function(module) { nit: blank after function and aModule. @@ +23,5 @@ > +}; > +const LOCAL_TEST_FOLDER = collector.addHttpResource("../../../data/downloading/"); > + > +var setupModule = function(module) { > + controller = mozmill.getBrowserController(); On another bug we are working to get the scope corrected. So please use aModule here otherwise a global variable is getting created. The same applies to the other created instances in this function. @@ +30,5 @@ > + pb.open(controller); > + > + // Download Indicator Buttons > + downloadButton = new elementslib.ID(controller.window.document, "downloads-indicator"); > + pbDownloadButton = new elementslib.ID(pb.controller.window.document, "downloads-indicator"); No, we do not want to see any of those lines in tests. Please also move that to the beforementioned ui class. @@ +41,5 @@ > + // Clean the Download Manager database > + dm.cleanAll(); > +} > + > +var teardownModule = function(module) { Same applies here for aModule. @@ +45,5 @@ > +var teardownModule = function(module) { > + // Clean all downloaded files from the system > + dm.cleanAll(); > + > + dm.resetDownloadLocation(DOWNLOAD_LOCATION); Why does reset need a parameter? @@ +54,5 @@ > + > +/** > + * Test that normal and pb downloads are kept separate > + */ > +var testDownloadIndicator = function() { You should choose a better name here to we know this is about downloads in combination with the private browsing mode. @@ +62,5 @@ > + DOWNLOADS.normal.forEach(function (aFile) { > + downloads.downloadFileOfUnknownType(controller, LOCAL_TEST_FOLDER + aFile); > + }); > + > + // Private Browsing Hm, where do we switch into PB mode? @@ +86,5 @@ > + > + // Check that number of pb downloaded files is identical to the original pb download list > + var PBIntersection = utils.arrayIntersection(downloadedPBFiles, DOWNLOADS.pb); > + assert.equal(PBIntersection.length, DOWNLOADS.pb.length, > + "Private Downloads are correctly shown in the Private Downloads Indicator Panel"); Not sure I understand the code above cause it always operates on a normal browser window. @@ +94,5 @@ > + * Get the temporary folder and create a subfolder based on a timestamp > + * @return {string} path to the newly created temporary folder > + */ > +function getTempDownloadLocation() { > + var downloadDir = Services.dirsvc.get("TmpD", Ci.nsIFile); I have a better idea here. Why can't we use %profile%/downloads? That would make sure Mozmill itself finally will clean-up everything even if a test misses a file. @@ +96,5 @@ > + */ > +function getTempDownloadLocation() { > + var downloadDir = Services.dirsvc.get("TmpD", Ci.nsIFile); > + downloadDir.append(new Date().getTime()); > + return downloadDir.path; nit: blank line before the return statement.

Uploaded with a feedback request since there might be a couple issues that need clarifying. > @@ +19,5 @@ > > var utils = require("utils"); > > > > const gTimeout = 5000; > > +const PREF_DOWNLOAD_LOCATION = "browser.download.dir"; > > +const PREF_DOWNLOAD_FOLDERLIST = "browser.download.folderList"; > > Just curious. What's that? Worth a comment here? I've added a comment line here. We need this to have the first pref used at all. See: https://developer.mozilla.org/en-US/docs/Download_Manager_preferences > @@ +189,3 @@ > > > > + function getDownloads(type) { > > + if (type === "private") > > Rather making it a string I think it would be better to use a boolean here, > e.g. aIsPrivate. I've changed it to a boolean. This makes the API less clear in my opinion. // Not clear that some are Normal and Some are private dm.getDownloads() vs dm.getDownloads(true) // Better (as it was before) dm.getDownloads() vs dm.getDownloads("private") // Maybe even better, but adds some (maybe unnecessary) complexity dm.getDownloads() vs dm.getDownlaods({ "private": true }) > @@ +423,5 @@ > > + * > > + * @param {MozMillController} controller MozMillController of the browser window > > + * @returns {array} list of downloaded file names > > + */ > > +function getPanelDownloads(aController) { > > This should be moved into an appropriate ui class instead of having it as a > global function in the downloads.js module. I've initially left this out of the class since it isn't really part of the DM, but then again, the "DM" as it initially was is no more, so we might as well integrate into the class. Done. > @@ +425,5 @@ > > + * @returns {array} list of downloaded file names > > + */ > > +function getPanelDownloads(aController) { > > + var button = new elementslib.ID(aController.window.document, "downloads-indicator"); > > + aController.click(button); > > Keep in mind that this will not close the opened popup. The Panel get closed when it loses focus. I've forced it now by pressing Escape. > @@ +431,5 @@ > > + getElementById("downloadsPanel")); > > + var downloadIndicatorItems = nodeCollector.queryNodes("richlistitem").nodes; > > + var downloadedFiles = []; > > + for (var i = 0; i < downloadIndicatorItems.length; i++) { > > + downloadedFiles.push(downloadIndicatorItems[i].attributes["target"].textContent); > > We can use map() here. Which other information are available? Switched to using map(). We actually have useful attributes: nodeValue:unknown_type_pb.stbf textContent:unknown_type_pb.stbf value:unknown_type_pb.stbf I have switched to using "value" since it might be more stable than textContent > ::: tests/functional/testPrivateBrowsing/testDownloadIndicator.js > @@ +9,5 @@ > > +var downloads = require("../../../lib/downloads"); > > +var privateBrowsing = require("../../../lib/ui/private-browsing"); > > +var utils = require("../../../lib/utils"); > > + > > +const DOWNLOAD_LOCATION = getTempDownloadLocation(); > > This function should be in utils.js. Moved. > @@ +17,5 @@ > > + "unknown_type.fmtd" > > + ], > > + pb: [ > > + "unknown_type_pb.stbf", > > + "unknown_type_pb.dets" > > Why this new file? So we have 2 private downloads, to be on par with the Normal Downloads > @@ +23,5 @@ > > +}; > > +const LOCAL_TEST_FOLDER = collector.addHttpResource("../../../data/downloading/"); > > + > > +var setupModule = function(module) { > > + controller = mozmill.getBrowserController(); > > On another bug we are working to get the scope corrected. So please use > aModule here otherwise a global variable is getting created. The same > applies to the other created instances in this function. We actually don't need this here anymore. Removed the param completely. > @@ +45,5 @@ > > +var teardownModule = function(module) { > > + // Clean all downloaded files from the system > > + dm.cleanAll(); > > + > > + dm.resetDownloadLocation(DOWNLOAD_LOCATION); > > Why does reset need a parameter? Refactored with a setter. Not sure if I took the best approach there, if the setter uses null (or an incorrect value) we reset the value to default. Should we still leave in a reset method, or is it ok that way? > @@ +54,5 @@ > > + > > +/** > > + * Test that normal and pb downloads are kept separate > > + */ > > +var testDownloadIndicator = function() { > > You should choose a better name here to we know this is about downloads in > combination with the private browsing mode. Renamed to testPrivateDownloadPanel > @@ +62,5 @@ > > + DOWNLOADS.normal.forEach(function (aFile) { > > + downloads.downloadFileOfUnknownType(controller, LOCAL_TEST_FOLDER + aFile); > > + }); > > + > > + // Private Browsing > > Hm, where do we switch into PB mode? Uhm, there is no more switch... We are using the private window controller. > @@ +86,5 @@ > > + > > + // Check that number of pb downloaded files is identical to the original pb download list > > + var PBIntersection = utils.arrayIntersection(downloadedPBFiles, DOWNLOADS.pb); > > + assert.equal(PBIntersection.length, DOWNLOADS.pb.length, > > + "Private Downloads are correctly shown in the Private Downloads Indicator Panel"); > > Not sure I understand the code above cause it always operates on a normal > browser window. Actually it operates on the private browsing window. We are basically making the same operations: * first on the normal window (controller) * then on the private window (pb.controller) > @@ +94,5 @@ > > + * Get the temporary folder and create a subfolder based on a timestamp > > + * @return {string} path to the newly created temporary folder > > + */ > > +function getTempDownloadLocation() { > > + var downloadDir = Services.dirsvc.get("TmpD", Ci.nsIFile); > > I have a better idea here. Why can't we use %profile%/downloads? That would > make sure Mozmill itself finally will clean-up everything even if a test > misses a file. That's a great idea. Thanks Henrik. Implemented it this way.

Comment on attachment 710595 [details] [diff] [review] patch v6 Review of attachment 710595 [details] [diff] [review]: ----------------------------------------------------------------- This is looking good. I have taken the request for feedback, but if possible would still like Henrik's thoughts before we land this. Please upload a new patch and ask for review from both myself and Henrik. Thanks Andrei! ::: lib/downloads.js @@ +19,5 @@ > var utils = require("utils"); > > const gTimeout = 5000; > > +// Directory where our downloads are saved I don't think this comment is necessary @@ +21,5 @@ > const gTimeout = 5000; > > +// Directory where our downloads are saved > +const PREF_DOWNLOAD_DIR = "browser.download.dir"; > +// Needs to be set to 2 (custom) for "browser.download.dir" to have any effect No need to repeat the preference name, perhaps "Needs to be set to 2 for custom download path to be used" @@ +180,5 @@ > + > + /** > + * Get a list of normal or private downloads > + * > + * @param {boolean} Type What downloads to return. Empty for all downloads, The jsdoc needs updating. @@ +184,5 @@ > + * @param {boolean} Type What downloads to return. Empty for all downloads, > + * "private" or "normal" to filter them accordingly > + * @returns {Array} List of downloads > + */ > + getDownloads : function downloadManager_getDownloads(aIsPrivate) { I agree that the string matching is not the best way to go, but also agree that the boolean doesn't make the test code particularly readable. Could we have two public methods here named getPublicDownloads and getPrivateDownloads? These could call a private method getDownloads, which takes the DBConnection object as an argument. @@ +341,5 @@ > + * @param {string} aPath Where to save downloaded files > + * If null or invalid value will reset to default setting > + */ > + set DownloadLocation(aPath) { > + if (aPath && typeof aPath === "string" && aPath.length > 0){ I don't think we should do this type checking here. The value should be a string. We should retain the resetDownloadLocation to clear the user preference instead of hijacking the setter. ::: lib/utils.js @@ +315,5 @@ > + * Get the profile folder and create a subfolder "downloads" > + * @return {string} path to the newly created folder > + */ > +function getProfileDownloadLocation() { > + var aDownloadDir = Services.dirsvc.get("ProfD", Ci.nsIFile); This doesn't need to be prefixed with 'a' as it's not an argument. Simply dir or downloadDir would be fine.

Thanks Dave for the feedback. Addressed issues, and some comments: > @@ +184,5 @@ > > + * @param {boolean} Type What downloads to return. Empty for all downloads, > > + * "private" or "normal" to filter them accordingly > > + * @returns {Array} List of downloads > > + */ > > + getDownloads : function downloadManager_getDownloads(aIsPrivate) { > > I agree that the string matching is not the best way to go, but also agree > that the boolean doesn't make the test code particularly readable. Could we > have two public methods here named getPublicDownloads and getPrivateDownloads? > These could call a private method getDownloads, which takes the DBConnection > object as an argument. I really like this. Thanks for the idea Dave :) > ::: lib/utils.js > @@ +315,5 @@ > > + * Get the profile folder and create a subfolder "downloads" > > + * @return {string} path to the newly created folder > > + */ > > +function getProfileDownloadLocation() { > > + var aDownloadDir = Services.dirsvc.get("ProfD", Ci.nsIFile); > > This doesn't need to be prefixed with 'a' as it's not an argument. Simply dir > or downloadDir would be fine. Whoa, just learned that our used "a" prefix comes from "argument". Finally makes sense why we use that (and where and how it should be used). Thanks!

Comment on attachment 713329 [details] [diff] [review] patch v7 Review of attachment 713329 [details] [diff] [review]: ----------------------------------------------------------------- Just one nit before we can land this. ::: lib/downloads.js @@ +200,5 @@ > + * > + * @param {mozIStorageConnection} dbConn Where downloads are stored > + * @returns {Array} List of downloads > + */ > + _getDownloads : function downloadManager_getDownloads(dbConn) { I'd suggest aDBConnection as the argument name.

Also, could you provide passing report URLs for each platform. Thanks! :)

Fixed nit. Testruns: OSX http://mozmill-crowd.blargon7.com/#/functional/report/a83c700664548dba07298b74bf65f3ee Linux http://mozmill-crowd.blargon7.com/#/functional/report/a83c700664548dba07298b74bf6fb935 WINDOWS http://mozmill-crowd.blargon7.com/#/functional/report/a83c700664548dba07298b74bf6fd9bf

fixed typo

A clean passing testrun on Linux: http://mozmill-crowd.blargon7.com/#/functional/report/a83c700664548dba07298b74bf732f23

Comment on attachment 714325 [details] [diff] [review] patch v8.1 Review of attachment 714325 [details] [diff] [review]: ----------------------------------------------------------------- Landed as: http://hg.mozilla.org/qa/mozmill-tests/rev/d2a1b3c07772

All tests passed over the weekend. Can be transplanted to mozilla-aurora (applies cleanly)

Transplanted to aurora: http://hg.mozilla.org/qa/mozmill-tests/rev/aeb216da1f33 (aurora)

(In reply to Andrei Eftimie from comment #42) > > @@ +17,5 @@ > > > + "unknown_type.fmtd" > > > + ], > > > + pb: [ > > > + "unknown_type_pb.stbf", > > > + "unknown_type_pb.dets" > > > > Why this new file? > > So we have 2 private downloads, to be on par with the Normal Downloads We selected a different count on purpose. So I would like to see that we continue with that. > Should we still leave in a reset method, or is it ok that way? I would love to see a reset method, yes. (In reply to Dave Hunt (:davehunt) from comment #45) > > + _getDownloads : function downloadManager_getDownloads(dbConn) { > > I'd suggest aDBConnection as the argument name. And the internal name should get two underscores given that it is a private method. (In reply to Dave Hunt (:davehunt) from comment #43) > This is looking good. I have taken the request for feedback, but if possible > would still like Henrik's thoughts before we land this. Please upload a new Looks like this review request got under the wire. Would you mind to file a follow-up bug for the remaining issues?

Comment on attachment 714325 [details] [diff] [review] patch v8.1 Review of attachment 714325 [details] [diff] [review]: ----------------------------------------------------------------- After digging through the patch I feel we should reopen this bug and not file a new one. There are substantial changes necessary. Not sure yet if we should back out the landed changesets or come up with follow-up patches. ::: lib/downloads.js @@ +181,5 @@ > + * Get the list of Public Downloads > + * > + * @return {Array} List of downloads > + */ > + getPublicDownloads : function downloadManager_getPublicDownloads() { This would have been perfect for a getter. @@ +190,5 @@ > + * Get the list of Private Downloads > + * > + * @return {Array} List of downloads > + */ > + getPrivateDownloads : function downloadManager_getPrivateDownloads() { Same here. @@ +198,5 @@ > + /** > + * Get a list of normal or private downloads > + * > + * @param {mozIStorageConnection} aDBConnection Where downloads are stored > + * @returns {Array} List of downloads Usually you use a syntax like `{[Object]}` @@ +309,5 @@ > + aController.click(button); > + > + // Collect the downloads from the UI > + var nodeCollector = new domUtils.nodeCollector(aController.window.document. > + getElementById("downloadsPanel")); Why do you circumvent the elementslib here? @@ +348,5 @@ > }, > > /** > + * Retrieve the currently set Download Location > + * @return {string} aPath where the downloaded files are saved Please keep the right capitalization: String vs. string. @@ +350,5 @@ > /** > + * Retrieve the currently set Download Location > + * @return {string} aPath where the downloaded files are saved > + */ > + get DownloadLocation() { Usually we put getters and setters first in the class definition. Also it should have a name as 'downloadLocation' or better 'downloadDir'. ::: tests/functional/testPrivateBrowsing/testPrivateDownloadPanel.js @@ +26,5 @@ > + > +var setupModule = function () { > + controller = mozmill.getBrowserController(); > + > + pb = new privateBrowsing.PrivateBrowsingWindow(); Missing aModule for the declaration. @@ +74,5 @@ > + var downloadedFiles = dm.getPanelDownloads(controller); > + > + // Check that number of normal downloaded files is identical to the original download list > + var intersection = utils.arrayIntersection(downloadedFiles, DOWNLOADS.normal); > + assert.equal(intersection.length, DOWNLOADS.normal.length, As I have mentioned before arrayIntersection() doesn't seem to be necessary here. Please use assert.deepEqual() instead. @@ +81,5 @@ > + // Open the Private Download Indicator and read the downloaded item list > + var downloadedPBFiles = dm.getPanelDownloads(pb.controller); > + > + // Check that number of pb downloaded files is identical to the original pb download list > + var PBIntersection = utils.arrayIntersection(downloadedPBFiles, DOWNLOADS.pb); Same here.

(In reply to Henrik Skupin (:whimboo) [away 02/09 - 02/017] from comment #54) > Comment on attachment 714325 [details] [diff] [review] > patch v8.1 > > Review of attachment 714325 [details] [diff] [review]: > ----------------------------------------------------------------- > > After digging through the patch I feel we should reopen this bug and not > file a new one. There are substantial changes necessary. Not sure yet if we > should back out the landed changesets or come up with follow-up patches. I've made a follow-up patch. Passing testrun: http://mozmill-crowd.blargon7.com/#/functional/report/f36358d058daf73ddbf781501633f061 > (In reply to Andrei Eftimie from comment #42) > > > @@ +17,5 @@ > > > > + "unknown_type.fmtd" > > > > + ], > > > > + pb: [ > > > > + "unknown_type_pb.stbf", > > > > + "unknown_type_pb.dets" > > > > > > Why this new file? > > > > So we have 2 private downloads, to be on par with the Normal Downloads > > We selected a different count on purpose. So I would like to see that we > continue with that. Ok, reverted the changes. It would be nice to know the purpose. > @@ +309,5 @@ > > + aController.click(button); > > + > > + // Collect the downloads from the UI > > + var nodeCollector = new domUtils.nodeCollector(aController.window.document. > > + getElementById("downloadsPanel")); > > Why do you circumvent the elementslib here? Refactored to use elementslib > @@ +348,5 @@ > > }, > > > > /** > > + * Retrieve the currently set Download Location > > + * @return {string} aPath where the downloaded files are saved > > Please keep the right capitalization: String vs. string. Done. Also changed throughout the test to ensure the right capitalization: String > @@ +74,5 @@ > > + var downloadedFiles = dm.getPanelDownloads(controller); > > + > > + // Check that number of normal downloaded files is identical to the original download list > > + var intersection = utils.arrayIntersection(downloadedFiles, DOWNLOADS.normal); > > + assert.equal(intersection.length, DOWNLOADS.normal.length, > > As I have mentioned before arrayIntersection() doesn't seem to be necessary > here. Please use assert.deepEqual() instead. assert has a "private" deepEqual method. Used that. Wondering if we should properly expose that, or remove the "private" underscore.

Comment on attachment 715485 [details] [diff] [review] followUp_v1 Review of attachment 715485 [details] [diff] [review]: ----------------------------------------------------------------- Just a few comments. ::: tests/functional/restartTests/testAddons_installFromFTP/test2.js @@ +39,5 @@ > > assert.ok(addonsManager.isAddonInstalled({addon: addon}), > "Extension '" + persisted.addon.id + > "' has been correctly installed"); > +} I'd say we leave this for now since Mario is taking care of that in bug 786306 and here we don't have changes to restart tests. ::: tests/functional/testPrivateBrowsing/testPrivateDownloadPanel.js @@ +27,1 @@ > controller = mozmill.getBrowserController(); We are going to need aModule here and in teardownModule.

Fixed nits :)

Comment on attachment 724337 [details] [diff] [review] FollowUp_v1.1 Review of attachment 724337 [details] [diff] [review]: ----------------------------------------------------------------- Looks fine now, but it's not applying cleanly.

Quick update. Applies cleanly now.

Comment on attachment 724390 [details] [diff] [review] FollowUp_v1.1 Review of attachment 724390 [details] [diff] [review]: ----------------------------------------------------------------- Pushed to default: http://hg.mozilla.org/qa/mozmill-tests/rev/abb82ac6a81d Can you please check aurora?

Aurora applies cleanly, and runs without issues: http://mozmill-crowd.blargon7.com/#/functional/report/2a6536e9db9f5f44ed48c585106855ee

Transplanted to aurora: http://hg.mozilla.org/qa/mozmill-tests/rev/4eeba59ba68d (aurora) Guess we're done now, thanks :)