445021 - Add more functions to toolkit/mozapps/downloads/tests/chrome/utils.js

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[Anoop Saldanha (Poona)]

Utility functions added - 
1. addDownload() - which adds a live download.
2. cleanUp() - a general cleanup function.

Comment 1

[Anoop Saldanha (Poona)]

The utils.js file which will be updated is this - http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/tests/chrome/utils.js

Comment 2

[Anoop Saldanha (Poona)]

Attachment: v1.0

Comment 3

[Anoop Saldanha (Poona)]

Attachment: v2.0

Added a new function - populateDM() for updating the dm with fake data.

Comment 4

[Anoop Saldanha (Poona)]

Attachment: v3.0

Added a new function - getDMWindow() that returns an instance of the dm window

Comment 5

[Shawn Wilsher :sdwilsh]

Comment on attachment 329437 [details] [diff] [review]
v3.0

>+// This function is used to add live downloads to the dm
>+function addDownload()
javadoc-style comment please

>+{
>+   function createURI(aObj) {
>+    let ios = Cc["@mozilla.org/network/io-service;1"].
>+              getService(Ci.nsIIOService);
>+    return (aObj instanceof Ci.nsIFile) ? ios.newFileURI(aObj) :
>+            ios.newURI(aObj, null, null);
>+  }
>+
>+  const nsIWBP = Ci.nsIWebBrowserPersist;
>+
>+  let dm = Cc["@mozilla.org/download-manager;1"].
>+           getService(nsIDownloadManager);
>+  // We clear out the database
>+  dm.DBConnection.executeSimpleSQL("DELETE FROM moz_downloads");
Why are we clearing out the DB in a generic add download method?  seems like a bad idea

>+  let dmFile = Cc["@mozilla.org/file/directory_service;1"].
>+               getService(Ci.nsIProperties).get("TmpD", Ci.nsIFile);
>+  dmFile.append("dm-ui-test.file");
>+  if (dmFile && dmFile.exists())
>+    dmFile.remove(false);
We really should have a random name generated here so callers can call it more than once.

>+// Final basic clean up function that can be called at the end of the test
>+function cleanUp()
javadoc-style please

>+{
>+  let dm = Cc["@mozilla.org/download-manager;1"].
>+           getService(nsIDownloadManager);
>+  // Clean the dm
>+  dm.DBConnection.executeSimpleSQL("DELETE FROM moz_downloads");
>+  let dmFile = Cc["@mozilla.org/file/directory_service;1"].
>+               getService(Ci.nsIProperties).get("TmpD", Ci.nsIFile);
>+  dmFile.append("dm-ui-test.file");
>+  if (dmFile && dmFile.exists())
>+    dmFile.remove(false);
not sure what this is for....

>+
>+  return;
no need for a return

We should also close the download manager window if it's open in this method

>+// This function is used to populate the dm with fake data
>+function populateDM()
javadoc-style comment please

>+  let db = dm.DBConnection;
you don't have a dm variable to access here...

>+  db.executeSimpleSQL("DELETE FROM moz_downloads");
We shouldn't be doing this in this method

>+  let rawStmt = db.createStatement(
>+    "INSERT INTO moz_downloads (name, source, target, startTime, endTime, " +
>+      "state, currBytes, maxBytes, preferredAction, autoResume) " +
>+    "VALUES (:name, :source, :target, :startTime, :endTime, :state, " +
>+      ":currBytes, :maxBytes, :preferredAction, :autoResume)");
>+  let stmt = Cc["@mozilla.org/storage/statement-wrapper;1"].
>+             createInstance(Ci.mozIStorageStatementWrapper)
>+  stmt.initialize(rawStmt);
>+  for each (let dl in DownloadData) {
>+    for (let prop in dl)
>+      stmt.params[prop] = dl[prop];
>+
>+  stmt.execute();
>+  }
weird spacing


>+  return;
no need for return

This method should really take an argument and not depend on global data to populate the download manager.

>+// Returns an instance to the download manager window
>+function getDMWindow()
javadoc-style comment please

Comment 6

[Anoop Saldanha (Poona)]

(In reply to comment #5)
> (From update of attachment 329437 [details] [diff] [review])
> >+// This function is used to add live downloads to the dm
> >+function addDownload()
> javadoc-style comment please

Will do that

> 
> >+{
> >+   function createURI(aObj) {
> >+    let ios = Cc["@mozilla.org/network/io-service;1"].
> >+              getService(Ci.nsIIOService);
> >+    return (aObj instanceof Ci.nsIFile) ? ios.newFileURI(aObj) :
> >+            ios.newURI(aObj, null, null);
> >+  }
> >+
> >+  const nsIWBP = Ci.nsIWebBrowserPersist;
> >+
> >+  let dm = Cc["@mozilla.org/download-manager;1"].
> >+           getService(nsIDownloadManager);
> >+  // We clear out the database
> >+  dm.DBConnection.executeSimpleSQL("DELETE FROM moz_downloads");
> Why are we clearing out the DB in a generic add download method?  seems like a
> bad idea

Will remove this line

> 
> >+  let dmFile = Cc["@mozilla.org/file/directory_service;1"].
> >+               getService(Ci.nsIProperties).get("TmpD", Ci.nsIFile);
> >+  dmFile.append("dm-ui-test.file");
> >+  if (dmFile && dmFile.exists())
> >+    dmFile.remove(false);
> We really should have a random name generated here so callers can call it more
> than once.

You mean the file name?  Will it matter.  As in every download now writes to the same file and we don't need the data in the file, unless there is some lock that prevents the simultaneous write to the same file. 

> >+// Final basic clean up function that can be called at the end of the test
> >+function cleanUp()
> javadoc-style please
> 
> >+{
> >+  let dm = Cc["@mozilla.org/download-manager;1"].
> >+           getService(nsIDownloadManager);
> >+  // Clean the dm
> >+  dm.DBConnection.executeSimpleSQL("DELETE FROM moz_downloads");
> >+  let dmFile = Cc["@mozilla.org/file/directory_service;1"].
> >+               getService(Ci.nsIProperties).get("TmpD", Ci.nsIFile);
> >+  dmFile.append("dm-ui-test.file");
> >+  if (dmFile && dmFile.exists())
> >+    dmFile.remove(false);
> not sure what this is for....

Yeah.  Even I am wondering the same thing :p

> 
> >+
> >+  return;
> no need for a return
> 
> We should also close the download manager window if it's open in this method
> 
> >+// This function is used to populate the dm with fake data
> >+function populateDM()
> javadoc-style comment please
> 
> >+  let db = dm.DBConnection;
> you don't have a dm variable to access here...

Taken care of

> 
> >+  db.executeSimpleSQL("DELETE FROM moz_downloads");
> We shouldn't be doing this in this method

Actually before I update the dm with fake data I clean the dm.

> 
> >+  let rawStmt = db.createStatement(
> >+    "INSERT INTO moz_downloads (name, source, target, startTime, endTime, " +
> >+      "state, currBytes, maxBytes, preferredAction, autoResume) " +
> >+    "VALUES (:name, :source, :target, :startTime, :endTime, :state, " +
> >+      ":currBytes, :maxBytes, :preferredAction, :autoResume)");
> >+  let stmt = Cc["@mozilla.org/storage/statement-wrapper;1"].
> >+             createInstance(Ci.mozIStorageStatementWrapper)
> >+  stmt.initialize(rawStmt);
> >+  for each (let dl in DownloadData) {
> >+    for (let prop in dl)
> >+      stmt.params[prop] = dl[prop];
> >+
> >+  stmt.execute();
> >+  }
> weird spacing
> 
> 
> >+  return;
> no need for return
> 
> This method should really take an argument and not depend on global data to
> populate the download manager.

Okay

> 
> >+// Returns an instance to the download manager window
> >+function getDMWindow()
> javadoc-style comment please
> 

Comment 7

[Anoop Saldanha (Poona)]

> >+{
> >+  let dm = Cc["@mozilla.org/download-manager;1"].
> >+           getService(nsIDownloadManager);
> >+  // Clean the dm
> >+  dm.DBConnection.executeSimpleSQL("DELETE FROM moz_downloads");
> >+  let dmFile = Cc["@mozilla.org/file/directory_service;1"].
> >+               getService(Ci.nsIProperties).get("TmpD", Ci.nsIFile);
> >+  dmFile.append("dm-ui-test.file");
> >+  if (dmFile && dmFile.exists())
> >+    dmFile.remove(false);
> not sure what this is for....

Actually inside the clean up I am deleting the local file - dm-ui-test.file that has been created using the addDownload() function.  First I am checking its existence before deleting the file from the file system.  Is this right? - 


let dmFile = Cc["@mozilla.org/file/directory_service;1"].
             getService(Ci.nsIProperties).get("TmpD", Ci.nsIFile);

if (dmFile && dmFile.exists())
  dmFile.remove(false);

Comment 8

[Anoop Saldanha (Poona)]

Attachment: v4.0

I still haven't figured out how I can generate a new name for the file for addDownload().

In populateDM() I am deleting the db before I populate the db with new dummy data.  That is the reason I am clearing the db in populateDM()

Comment 9

[Shawn Wilsher :sdwilsh]

(In reply to comment #6)
> > >+  let dmFile = Cc["@mozilla.org/file/directory_service;1"].
> > >+               getService(Ci.nsIProperties).get("TmpD", Ci.nsIFile);
> > >+  dmFile.append("dm-ui-test.file");
> > >+  if (dmFile && dmFile.exists())
> > >+    dmFile.remove(false);
> > We really should have a random name generated here so callers can call it more
> > than once.
> 
> You mean the file name?  Will it matter.  As in every download now writes to
> the same file and we don't need the data in the file, unless there is some lock
> that prevents the simultaneous write to the same file. 
I don't think it's a good idea to try and write to the same file with more than one writer.  Our code never expects that, and I can see errors happening real fast.

(In reply to comment #7)
> Actually inside the clean up I am deleting the local file - dm-ui-test.file
> that has been created using the addDownload() function.  First I am checking
> its existence before deleting the file from the file system.  Is this right?
You should be deleting that in a download listener for any test file that adds a download then.

(In reply to comment #8)
> I still haven't figured out how I can generate a new name for the file for
> addDownload().
http://www.mediacollege.com/internet/javascript/number/random.html

> In populateDM() I am deleting the db before I populate the db with new dummy
> data.  That is the reason I am clearing the db in populateDM()
But what if the consumer already has data they want to keep.  The name populateDM doesn't imply that it's going to clear out any existing data.  I'd prefer to make consumers call cleanUp before they call populateDM if they want pristine state.

Comment 10

[Shawn Wilsher :sdwilsh]

Comment on attachment 329613 [details] [diff] [review]
v4.0

>+/**
>+ * Adds a live download to the download manager.
>+ *
>+ * @param aName
>+ *        A string that holds the name of the file that should be downloaded
>+ *        from the local http server.
>+ * @return Instance of the download(nsIDownload), created using the function
>+ *         nsIDownloadManager:addDownload()
>+ */
nit: general format here is
 * @returns an instance....
I'd just say "returns an instance of nsIDownload"

>+  let fileName = aName || "httpd.js";
>+  let dl = dm.addDownload(Ci.nsIDownloadManager.DOWNLOAD_TYPE_DOWNLOAD,
>+                          createURI("http://example.com/" + fileName),
>+                          createURI(dmFile), null, null,
>+                          Math.round(Date.now() * 1000), null, persist);
I think it'd be best to take a full URI here, or default to the httpd server

>+/**
>+ * A clean up function that can be called at the end of every chrome test.
>+ */
>+function cleanUp()
it might be better to call things something else like setCleanState

Please address the other comments I just posted as well.

Comment 11

[Anoop Saldanha (Poona)]

Attachment: v5.0

utils.js updated.

- Cleaning up of db in populateDM() removed.
- cleanUp() renamed to setCleanState()
- addDownload() update to create a random file name.

All the corresponding tests now updated to accomodate the new utils.js

The downloads that are added using addDownload() are now deleted within the test file itself.

Comment 12

[Shawn Wilsher :sdwilsh]

Comment on attachment 331507 [details] [diff] [review]
v5.0

r=sdwilsh, with comments addressed (attaching shortly)

Comment 13

[Shawn Wilsher :sdwilsh]

Attachment: v5.1

Comment 14

[Shawn Wilsher :sdwilsh]

Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/index.cgi/rev/bd0013e0d3af