Closed
Bug 445021
Opened 16 years ago
Closed 16 years ago
Add more functions to toolkit/mozapps/downloads/tests/chrome/utils.js
Categories
(Toolkit :: Downloads API, defect)
Toolkit
Downloads API
Tracking
()
RESOLVED
FIXED
People
(Reporter: poonaatsoc, Assigned: poonaatsoc)
References
Details
Attachments
(1 file, 5 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review |
Utility functions added -
1. addDownload() - which adds a live download.
2. cleanUp() - a general cleanup function.
Assignee | ||
Comment 1•16 years ago
|
||
The utils.js file which will be updated is this - http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/tests/chrome/utils.js
Assignee | ||
Comment 2•16 years ago
|
||
Attachment #329323 -
Flags: review?(sdwilsh)
Assignee | ||
Comment 3•16 years ago
|
||
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)
Assignee | ||
Comment 4•16 years ago
|
||
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)
Updated•16 years ago
|
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 5•16 years ago
|
||
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-
Assignee | ||
Comment 6•16 years ago
|
||
(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
>
Assignee | ||
Comment 7•16 years ago
|
||
> >+{
> >+ 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);
Assignee | ||
Comment 8•16 years ago
|
||
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)
Comment 9•16 years ago
|
||
(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•16 years ago
|
||
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-
Assignee | ||
Comment 11•16 years ago
|
||
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 12•16 years ago
|
||
Comment on attachment 331507 [details] [diff] [review]
v5.0
r=sdwilsh, with comments addressed (attaching shortly)
Attachment #331507 -
Flags: review?(sdwilsh) → review+
Comment 13•16 years ago
|
||
Attachment #331507 -
Attachment is obsolete: true
Comment 14•16 years ago
|
||
Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/index.cgi/rev/bd0013e0d3af
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Updated•16 years ago
|
Product: Firefox → Toolkit
You need to log in
before you can comment on or make changes to this bug.
Description
•