Closed Bug 445021 Opened 12 years ago Closed 12 years ago

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

Categories

(Toolkit :: Downloads API, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: poonaatsoc, Assigned: poonaatsoc)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 5 obsolete files)

Utility functions added - 
1. addDownload() - which adds a live download.
2. cleanUp() - a general cleanup function.
Attached patch v1.0 (obsolete) — Splinter Review
Attachment #329323 - Flags: review?(sdwilsh)
Attached patch v2.0 (obsolete) — Splinter Review
Added a new function - populateDM() for updating the dm with fake data.
Attachment #329323 - Attachment is obsolete: true
Attachment #329378 - Flags: review?(sdwilsh)
Attachment #329323 - Flags: review?(sdwilsh)
Attached patch v3.0 (obsolete) — Splinter Review
Added a new function - getDMWindow() that returns an instance of the dm window
Attachment #329378 - Attachment is obsolete: true
Attachment #329437 - Flags: review?(sdwilsh)
Attachment #329378 - Flags: review?(sdwilsh)
Summary: Updating http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/tests/chrome/utils.js with utility functions for the chrome tests → Add more functions to toolkit/mozapps/downloads/tests/chrome/utils.js
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
Attachment #329437 - Flags: review?(sdwilsh) → review-
(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
> 

> >+{
> >+  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);

Attached patch v4.0 (obsolete) — Splinter Review
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()
Attachment #329437 - Attachment is obsolete: true
Attachment #329613 - Flags: review?(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 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.
Attachment #329613 - Flags: review?(sdwilsh) → review-
Attached patch v5.0 (obsolete) — Splinter Review
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.
Attachment #329613 - Attachment is obsolete: true
Attachment #331507 - Flags: review?(sdwilsh)
Comment on attachment 331507 [details] [diff] [review]
v5.0

r=sdwilsh, with comments addressed (attaching shortly)
Attachment #331507 - Flags: review?(sdwilsh) → review+
Attached patch v5.1Splinter Review
Attachment #331507 - Attachment is obsolete: true
Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/index.cgi/rev/bd0013e0d3af
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.