This should be a chrome test that lives in toolkit/mozapps/downloads/tests/chrome/ to replace this litmus test: https://litmus.mozilla.org/show_test.cgi?id=3978

Sorry, this should be a browser test in browser/base/content/test/downloads

Comment on attachment 323875 [details] [diff] [review] Patch for bringing to focus an already open dm When you post a patch, make sure you request review from me please.

Handles both cases - dm already open and dm not yet open

Comment on attachment 323919 [details] [diff] [review] Patch_v2.0 >+# The Initial Developer of the Original Code is >+# Mozilla Corporation. This should be you, no? >+# Contributor(s): >+# Shawn Wilsher <me@shawnwilsher.com> (Original Author) Don't think I contributed ;) >diff --git a/browser/base/content/test/downloads/open_download_manager.js b/browser/base/content/test/downloads/open_download_manager.js >+ * The Initial Developer of the Original Code is >+ * Mozilla Corporation. Ditto >+var wm = Cc["@mozilla.org/appshell/window-mediator;1"]. >+ getService(Ci.nsIWindowMediator); >+var dm_win = wm.getMostRecentWindow("Download:Manager"); >+var dmui; >+const DLMGR_UI_DONE = "download-manager-ui-done"; >+if ( dm_win ) dm_win.close(); Please don't put these in the global scope >+function test() >+{ >+ waitForExplicitFinish(); usually this goes at the end >+ let obs = Cc["@mozilla.org/observer-service;1"].getService(Ci.nsIObserverService); >+ let testObs = { >+ observe: function(aSubject, aTopic, aData) { nit: indent here too please >+ if (aTopic == DLMGR_UI_DONE) { >+ // The very fact that aTopic is DLMGR_UI_DONE is the indication that the dm has been rendered on the screen >+ is( "true", ""); >+ aSubject.close(); >+ finish(); >+ } >+ } >+ }; >+ /** The lines right below test if the dm gains focus, when it is already open and when you try to open it again >+ */ >+ dmui = displayDMUI(); >+ setTimeout( "let a", 1); not sure what this is supposed to do >+ dm_win = dmui.recentWindow; This won't do what you expect it to do >+function openDMWithCtrlKey(var browserWin) >+{ >+ if( navigator.platform.search("Linux") == 0) { >+ EventUtils.synthesizeKey("j", { ctrlKey: true }, browserWin); >+ } >+ else if( navigator.platform.search("Mac") == 0 ) { >+ EventUtils.synthesizeKey("y", { metaKey: true }, browserWin); >+ } >+ else { >+ EventUtils.synthesizeKey("y", { ctrlKey: true }, browserWin); >+ } >+} Note, JavaScript style guidelines http://developer.mozilla.org/en/docs/JavaScript_style_guide Also, I'd rather see something like: function getOpenKeys() { return ["j", { metaKey: true}]; } but with the platform checks. Then just use the returned array members in your one synthesizeKey call. >+function displayDMUI() >+{ >+ dmui = Cc["@mozilla.org/download-manager-ui;1"]. >+ getService(Ci.nsIDownloadManagerUI); >+ dmui.show(); >+ return dmui; This will return the service, but it will only have the methods in the idl available.

> >+var wm = Cc["@mozilla.org/appshell/window-mediator;1"]. > >+ getService(Ci.nsIWindowMediator); > >+var dm_win = wm.getMostRecentWindow("Download:Manager"); > >+var dmui; > >+const DLMGR_UI_DONE = "download-manager-ui-done"; > >+if ( dm_win ) dm_win.close(); > Please don't put these in the global scope > Will have this under local scope > >+function test() > >+{ > >+ waitForExplicitFinish(); > usually this goes at the end > Actually I had a reason for this. I will get back to you on this :) > >+ let obs = Cc["@mozilla.org/observer-service;1"].getService(Ci.nsIObserverService); > >+ let testObs = { > >+ observe: function(aSubject, aTopic, aData) { > nit: indent here too please > Will have it done > >+ if (aTopic == DLMGR_UI_DONE) { > >+ // The very fact that aTopic is DLMGR_UI_DONE is the indication that the dm has been rendered on the screen > >+ is( "true", ""); > >+ aSubject.close(); > >+ finish(); > >+ } > >+ } > >+ }; > > > >+ /** The lines right below test if the dm gains focus, when it is already open and when you try to open it again > >+ */ > >+ dmui = displayDMUI(); > >+ setTimeout( "let a", 1); > not sure what this is supposed to do Actually I am waiting for a second so that dm ui can be rendered on the screen and then I can retrieve an instance to the dm window which is the next statement in this case dmui.recentWindow > > >+ dm_win = dmui.recentWindow; > This won't do what you expect it to do Won't this retrieve an instance to the dm win that is open? That is what d From nsDownloadManagerUI.js - get recentWindow() { 139 var wm = Cc["@mozilla.org/appshell/window-mediator;1"]. 140 getService(Ci.nsIWindowMediator); 141 return wm.getMostRecentWindow("Download:Manager"); 142 }, > > >+function openDMWithCtrlKey(var browserWin) > >+{ > >+ if( navigator.platform.search("Linux") == 0) { > >+ EventUtils.synthesizeKey("j", { ctrlKey: true }, browserWin); > >+ } > >+ else if( navigator.platform.search("Mac") == 0 ) { > >+ EventUtils.synthesizeKey("y", { metaKey: true }, browserWin); > >+ } > >+ else { > >+ EventUtils.synthesizeKey("y", { ctrlKey: true }, browserWin); > >+ } > >+} > > Note, JavaScript style guidelines > http://developer.mozilla.org/en/docs/JavaScript_style_guide > Also, I'd rather see something like: > function getOpenKeys() > { > return ["j", { metaKey: true}]; > } > but with the platform checks. Then just use the returned array members in your > one synthesizeKey call. > Okay > > >+function displayDMUI() > >+{ > >+ dmui = Cc["@mozilla.org/download-manager-ui;1"]. > >+ getService(Ci.nsIDownloadManagerUI); > >+ dmui.show(); > >+ return dmui; > This will return the service, but it will only have the methods in the idl > available. > Will get back to you on this too

(In reply to comment #6) > Actually I am waiting for a second so that dm ui can be rendered on the screen > and then I can retrieve an instance to the dm window which is the next > statement in this case dmui.recentWindow I don't think that does what you want it to do then. If you want to know when the window is open, you should listen to the observer topic. That's why we added it in the first place :) > Won't this retrieve an instance to the dm win that is open? That is what d It will retrieve an instance of nsIDownloadManagerUI, which only has the methods exposed by the idl: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/public/nsIDownloadManagerUI.idl I'd really like it if this followed more closely with the other download manager browser tests where you have a series of functions that test specific behavior. See http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/test/browser/browser_nsIDownloadManagerUI.js

Made the changes you had specified

Had the keys "y" "j" reveresed in the previous patch. Made the corrections :)

(In reply to comment #7) > (In reply to comment #6) > > Actually I am waiting for a second so that dm ui can be rendered on the screen > > and then I can retrieve an instance to the dm window which is the next > > statement in this case dmui.recentWindow > I don't think that does what you want it to do then. If you want to know when > the window is open, you should listen to the observer topic. That's why we > added it in the first place :) Missed your reply when I was updating the new patch :) Instead of waiting for the observer can I just do this displayDMUI(); dm_win = wm.getMostRecentWindow("Download:Manager"); Otherwise I will have to do some restructing of the code if I am going to use the observer

(In reply to comment #10) > Instead of waiting for the observer can I just do this > > displayDMUI(); > dm_win = wm.getMostRecentWindow("Download:Manager"); > > Otherwise I will have to do some restructing of the code if I am going to use > the observer It won't work because the window opens asynchronously. Also, are you running this test to make sure that it works? I suspect it doesn't work

Some restructuring done

Comment on attachment 324068 [details] [diff] [review] Patch_v5.0 >+ if ( dm_win ) dm_win.close(); nit: no space after ( or before ) >+ // The very fact that aTopic is DLMGR_UI_DONE is the indication that the dm has been rendered on the screen nit: line wrapping at 80 characters >+ is( "true", ""); don't need to actually check anything here. The test would time out anyway (that, and this paticular check, as written, will always fail). >+ obs.removeObserver( testObs, DLMGR_UI_DONE); nit: weird spacing >+function openDMWithCtrlKey(var browserWin) >+{ >+ let key, event; >+ [key, event] = getOpenKeys(); >+ if( navigator.platform.search("Linux") == 0) { >+ EventUtils.synthesizeKey( key, event, browserWin); >+ } >+ else if( navigator.platform.search("Mac") == 0 ) { >+ EventUtils.synthesizeKey( key, event, browserWin); >+ } >+ else { >+ EventUtils.synthesizeKey( key, event, browserWin); >+ } >+} you just need one call here to EventUtils.synthesizeKey(key, event). window should be correct here, so you shouldn't need to pass in anything. I don't think this test passes as is. Please make sure you test it before submitting it for review.

I have made the necessary changes. Please do check the contents of Makefile.in

Comment on attachment 325099 [details] [diff] [review] Patch_v6.0 >diff --git a/browser/base/content/test/downloads/Makefile.in b/browser/base/content >+# The Initial Developer of the Original Code is >+# Mozilla Corporation. Not really >+# Contributor(s): >+# Anoop Saldanha <poonaatsoc@gmail.com> (Original Author) nit: tab >diff --git a/browser/base/content/test/downloads/open_download_manager.js b/browser/base/content/test/downloads/open_download_manager.js >+ * The Initial Developer of the Original Code is Anoop Saldanha <poonaatsoc@gmail.com> >+ * Mozilla Corporation. your name should be on a new line, and get rid of the Mozilla Corporation >+function getOpenKeys() >+{ >+ if(navigator.platform.search("Linux") == 0) { >+ return ["y", { ctrlKey: true }]; >+ } nit: no braces for one line ifs >+ else if(navigator.platform.search("Mac") == 0 ) { >+ return ["j", { metaKey: true }]; >+ } ditto >+ else { >+ return ["j", { ctrlKey: true }]; >+ } ditto >\ No newline at end of file fix please You also have tabs throughout your file. Please convert those to spaces.

> >+# Contributor(s): > >+# Anoop Saldanha <poonaatsoc@gmail.com> (Original Author) > nit: tab Darn! I must have changed my editor settings. > >+ * Mozilla Corporation. > your name should be on a new line, and get rid of the Mozilla Corporation > Okay > >+function getOpenKeys() > >+{ > >+ if(navigator.platform.search("Linux") == 0) { > >+ return ["y", { ctrlKey: true }]; > >+ } > nit: no braces for one line ifs I missed this completely. > > >\ No newline at end of file > fix please Okay

Made the changes. One additional change that I made is I added a new function so that I could separate the two tests. Now someone looking at the code can easily make out that the code holds 2 tests within it. I read the article on building this test. I am not very confident about it. Would prefer having some help over there. Also I have started coding other tests. I will submit the patches for those once this patch gets through

Comment on attachment 326163 [details] [diff] [review] Patch_v7.0 >+++ b/browser/base/content/test/downloads/open_download_manager.js >+ * The Original Code is mozilla.org This could be more descriptive like download manager test code. >+function test() Could you put a /** * */ comment block describing what the test is supposed to do? >+ if (dm_win) dm_win.close(); General styling nit: single line if blocks still have their body on the next line indented. >+ let testObs = { >+ observe: function(aSubject, aTopic, aData) { >+ obs.removeObserver(testObs, DLMGR_UI_DONE); To be safe, perhaps you should check the topic coming in and make sure it's not some other notification before doing work >+ //Call the focus test straight away General styling nit: space after the // and before the comment >+//Tests if a a dm is rendered on the screen when you try to open it with a Ctrl-[key] nit: Fix the comment (typo and description [Ctrl]) >+function testDMRender() >+{ >+ openDMWithCtrlKey(); >+} How does this function test if the DM is rendered? It's just calls the open function, but doesn't do any checks. >+//Tests if an already open but de focussed dm gains focus when you attempt open it >+function testDMFocus() >+{ >+ // Defocus the DM, so that you can carry out your tests on it >+ dm_win.blur(); If you're going to do the dmui.visible test below, probably have one before and after the blur() call to make sure it actually blurs. >+ //Trying to open an already open dm should bring the dm to focus >+ openDMWithCtrlKey(); >+ ok(dmui.visible, "Rendering test for dm passed, but the DM focus test failed"); How does this know the DM focus test failed? >+ >+ General styling nit: no double empty lines >+function openDMWithCtrlKey() >+{ >+ let key, event; >+ [key, event] = getOpenKeys(); nit: You can just do |let [key, event] = ...| on a single line >+function getOpenKeys() >+{ >+ if (navigator.platform.search("Linux") == 0) return ["y", { ctrlKey: true }]; >+ else if (navigator.platform.search("Mac") == 0) return ["j", { metaKey: true }]; >+ else return ["j", { ctrlKey: true }]; You don't need the last else because everything else already returns and quits the function. Also, if body statements on new line.

(In reply to comment #18) > How does this function test if the DM is rendered? It's just calls the open > function, but doesn't do any checks. The test will time out of it doesn't. There's nothing to really check because the test will continue on (with failures or passes), or it will time out. > If you're going to do the dmui.visible test below, probably have one before and > after the blur() call to make sure it actually blurs. The problem is that window.focus and window.blur are async - I need to figure out how to deal with that in another bug too.

(In reply to comment #18) > (From update of attachment 326163 [details] [diff] [review]) > >+++ b/browser/base/content/test/downloads/open_download_manager.js > >+ * The Original Code is mozilla.org > This could be more descriptive like download manager test code. Taken care > >+function test() > Could you put a /** * */ comment block describing what the test is supposed to > do? Ah. Actually I had done this for other tests. Will do it here also > >+ if (dm_win) dm_win.close(); > General styling nit: single line if blocks still have their body on the next > line indented. I am sorry, I didn't get you here. > > >+ let testObs = { > >+ observe: function(aSubject, aTopic, aData) { > >+ obs.removeObserver(testObs, DLMGR_UI_DONE); > To be safe, perhaps you should check the topic coming in and make sure it's not The reason I didn't check the topic coming in is because, testObs is listening to only DLMGR_UI_DONE. It isn't registered to listen to anything else. That is why I am not checking for the topic. Adding the check would just be a formality then. Do I need to add the check? > some other notification before doing work > > >+ //Call the focus test straight away > General styling nit: space after the // and before the comment Fixed > > >+//Tests if a a dm is rendered on the screen when you try to open it with a Ctrl-[key] > nit: Fix the comment (typo and description [Ctrl]) > >+function testDMRender() > >+{ > >+ openDMWithCtrlKey(); > >+} > How does this function test if the DM is rendered? It's just calls the open > function, but doesn't do any checks. > Actually the ok( ) call is placed in testDMFocus(), since testDMFocus()'s ok() is able to fathom out the results for both these tests. I and shawn had discussed this, and it was decided that we use only the testDMFocus()'s ok( ) call to conclude if both testDMRender() and testDMFocus() failed or not. > >+//Tests if an already open but de focussed dm gains focus when you attempt open it > >+function testDMFocus() > >+{ > >+ // Defocus the DM, so that you can carry out your tests on it > >+ dm_win.blur(); > If you're going to do the dmui.visible test below, probably have one before and > after the blur() call to make sure it actually blurs. > As shawn told it works asyn. So I am not really sure if I should wait for a couple of seconds with a timer(hoping that it blurs within the timer) to check for dmui.visisble, but I am not really certain if that is going to work since it works async. > >+ //Trying to open an already open dm should bring the dm to focus > >+ openDMWithCtrlKey(); > >+ ok(dmui.visible, "Rendering test for dm passed, but the DM focus test failed"); > How does this know the DM focus test failed? testDMFocus() is called from the observer only if the dm has been rendered on the screen which is an indication that testDMRender() passed, that means testDMFocus() relies on the testDMRender() passing. Now if testDMFocus fails then the error message is printed - i.e. the render test passed but the focus test failed. If both pass, that error message is not printed. If testDMRender doesn't pass, then the observer is never called and even testDMFocus() is never called and hence the test times out which means a failure for both these tests. > >+ > >+ > General styling nit: no double empty lines > > >+function openDMWithCtrlKey() > >+{ > >+ let key, event; > >+ [key, event] = getOpenKeys(); > nit: You can just do |let [key, event] = ...| on a single line > Taken care > >+function getOpenKeys() > >+{ > >+ if (navigator.platform.search("Linux") == 0) return ["y", { ctrlKey: true }]; > >+ else if (navigator.platform.search("Mac") == 0) return ["j", { metaKey: true }]; > >+ else return ["j", { ctrlKey: true }]; > You don't need the last else because everything else already returns and quits > the function. The last "else" is for windows. I am not not really sure if I have interpreted you right here. > Also, if body statements on new line. I didn't get this one either Updating a couple of changes you requested for, in the next version of the patch v8.0

Made a couple of changes that edward asked for. I didn't understand a couple of comments. Please have a check and let me know. The tests that I am currently working in is listed here - http://wiki.mozilla.org/User:Poona/litmus_tests/dm I will be adding more tests today.

Comment on attachment 326487 [details] [diff] [review] Patch_v8.0 >+++ b/browser/base/content/test/downloads/Makefile.in >+# Contributor(s): >+# >+# nit: There's an extra line here that shouldn't be there. >+++ b/browser/base/content/test/downloads/open_download_manager.js >+ * Contributor(s): >+ * >+ * ditto >+/* *****Automation of litmus test - Testcase ID #3978 - Opening the Downloads dialog***** nit: open comments like this up like this: /** * {title} nit: line wrapping at 80 characters >+ * Each of the actions should open the Downloads manager dialog if not already open or bring nit: line wrapping at 80 characters >+ * it into focus if it was already open. >+ * nit: trailing empty line not needed >+ >+ >+ nit: way too much space >+function test() >+{ >+ const DLMGR_UI_DONE = "download-manager-ui-done"; >+ >+ let wm = Cc["@mozilla.org/appshell/window-mediator;1"]. >+ getService(Ci.nsIWindowMediator); >+ let dm_win = wm.getMostRecentWindow("Download:Manager"); >+ if (dm_win) dm_win.close(); >+ >+ let obs = Cc["@mozilla.org/observer-service;1"]. >+ getService(Ci.nsIObserverService); >+ let testObs = { >+ observe: function(aSubject, aTopic, aData) { >+ obs.removeObserver(testObs, DLMGR_UI_DONE); add comment saying that at this point we know the window was opened >+ >+ nit: just one line between functions please >+// Tests if a dm is rendered on the screen when you try to open it with a [Ctrl/Meta]-[y/m] nit: line wrapping at 80 characters nit: for descriptions above comments, please use javadoc style headers: /** * Tests... */ >+function testDMRender() place this in the proper execution order please >+function testDMFocus() >+{ >+ let wm = Cc["@mozilla.org/appshell/window-mediator;1"]. >+ getService(Ci.nsIWindowMediator); >+ let dmui = Cc["@mozilla.org/download-manager-ui;1"]. >+ getService(Ci.nsIDownloadManagerUI); >+ let dm_win = wm.getMostRecentWindow("Download:Manager"); >+ // Defocus the DM, so that you can carry out your tests on it >+ dm_win.blur(); >+ // Trying to open an already open dm should bring the dm to focus >+ openDMWithCtrlKey(); >+ ok(dmui.visible, "Rendering test for dm passed, but the DM focus test failed"); >+ dm_win.close(); You need to listen for focus and blur events here because both are async events. >+function openDMWithCtrlKey() nit: function name inaccurate. How about openDMWithKey? >+ if (navigator.platform.search("Linux") == 0) return ["y", { ctrlKey: true }]; if (navigator.platform.search("Linux") == 0) return ["y", { ctrlKey: true }]; >+ else if (navigator.platform.search("Mac") == 0) return ["j", { metaKey: true }]; ditto >+ else return ["j", { ctrlKey: true }]; ditto Overall, this test doesn't seem complete. You don't test the bottom bar, and you don't test the menu (although the latter I'm unsure that you can actually test it due to platform inconsistencies - might still need to be a litmus test)

I have made the update for the statusbar click, but not Tools|Downloads. I am not sure about whether a click on Tools|Downloads can be done. The tests got a bit complex though. I have added listener for blur and focus events and then carried out the checks.

Comment on attachment 329440 [details] [diff] [review] Patch_v11.0 global nit: please use javadoc-style comments for function headers. >+++ b/browser/base/content/test/downloads/Makefile.in >+# nit: no extra space at top please >+++ b/browser/base/content/test/downloads/open_download_manager.js >+var ios = Cc["@mozilla.org/network/io-service;1"].getService(Ci.nsIIOService); >+var dmFile = Cc["@mozilla.org/file/directory_service;1"]. >+ getService(Ci.nsIProperties).get("TmpD", Ci.nsIFile); >+dmFile.append("dm-ui-test.file"); >+dmFile.createUnique(Ci.nsIFile.NORMAL_FILE_TYPE, 0666); >+var gTestPath = ios.newFileURI(dmFile).spec; >+ >+const DownloadData = [ >+ { name: "Firefox 2.0.0.11.dmg", >+ source: "http://mozilla-mirror.naist.jp//firefox/releases/2.0.0.11/mac/en-US/Firefox%202.0.0.11.dmg", >+ target: gTestPath, >+ startTime: 1200185939538521, >+ endTime: 1200185939538521, >+ entityID: "%22216c12-1116bd8-440070d5d2700%22/17918936/Thu, 29 Nov 2007 01:15:40 GMT", >+ state: Ci.nsIDownloadManager.DOWNLOAD_DOWNLOADING, >+ currBytes: 0, maxBytes: -1, preferredAction: 0, autoResume: 0 }, >+]; Do we even need download data for this test? >+function openDMWithKey() >+{ >+ let [key, event] = getOpenKeys(); >+ EventUtils.synthesizeKey(key, event, window); don't need to pass in window >+function testDMFocus(aWin) >+ var dmui = Cc["@mozilla.org/download-manager-ui;1"]. >+ getService(Ci.nsIDownloadManagerUI); not sure why you have this here >+function testClickStatusBarDMIcon(aWin) >+{ >+ function dmWindowBlurred() { >+ EventUtils.synthesizeMouse(download-monitor, 0, 0, { }, window); no need for window should remove the blur listener here >+ } >+ >+ function dmWindowFocused() { >+ // Test if the dmui is visible >+ ok(dmui.visible, "Clicking on the download monitor on the statusbar doesn't " + >+ + "bring the dm to focus"); note: the text in ok and is are the success messages, not the failure ones also, dmui isn't defined here >+ var dmui = Cc["@mozilla.org/download-manager-ui;1"]. >+ getService(Ci.nsIDownloadManagerUI); >+ var dm = Cc["@mozilla.org/download-manager;1"]. >+ getService(Ci.nsIDownloadManager); >+ var db = dm.DBConnection; >+ >+ // First, we populate the database with some fake data >+ db.executeSimpleSQL("DELETE FROM moz_downloads"); >+ var rawStmt = db.createStatement( >+ "INSERT INTO moz_downloads (name, source, target, startTime, endTime, " + >+ "state, currBytes, maxBytes, preferredAction, autoResume, entityID) " + >+ "VALUES (:name, :source, :target, :startTime, :endTime, :state, " + >+ ":currBytes, :maxBytes, :preferredAction, :autoResume, :entityID)"); >+ var stmt = Cc["@mozilla.org/storage/statement-wrapper;1"]. >+ createInstance(Ci.mozIStorageStatementWrapper) >+ stmt.initialize(rawStmt); >+ for each (var dl in DownloadData) { >+ for (var prop in dl) >+ stmt.params[prop] = dl[prop]; >+ >+ stmt.execute(); >+ } >+ stmt.statement.finalize(); >+ >+ var download_monitor = window.document.getElementById("download-monitor"); I don't even know why this code is here It seems pretty clear that this test wouldn't pass in it's current state. Please make sure you run tests and that they pass before requesting review. It takes me a good deal of time to review code, and it makes me sad to know that the code I'm reviewing doesn't even work.

I have tested this as a chrome test and it passes.

Comment on attachment 336629 [details] [diff] [review] v12.0 Can you use mozIJSSubScriptLoader to load the util functions in toolkit here please? The magic incantation is going to look something like this: let DownloadTestUtils; Cc["@mozilla.org/moz/jssubscript-loader;1"]. getService(mozIJSSubScriptLoader). loadSubscript("path/to/file", DownloadTestUtils); r=sdwilsh with that change