Closed
Bug 380250
Opened 17 years ago
Closed 17 years ago
Convert Download Manager's RDF backend to mozStorage
Categories
(Toolkit :: Downloads API, defect)
Toolkit
Downloads API
Tracking
()
VERIFIED
FIXED
mozilla1.9alpha4
People
(Reporter: sdwilsh, Assigned: sdwilsh)
References
Details
Attachments
(4 files, 14 obsolete files)
5.05 KB,
patch
|
Details | Diff | Splinter Review | |
24.73 KB,
patch
|
Details | Diff | Splinter Review | |
29.92 KB,
patch
|
Details | Diff | Splinter Review | |
134.67 KB,
patch
|
Details | Diff | Splinter Review |
This first patch is just really to show brahmana what is planned. I've got a bunch of things that I've got written down on paper that needs to be fixed before I can even think of asking for review.
Comment 1•17 years ago
|
||
Comment on attachment 264342 [details] [diff] [review] v0.1 >+ NS_ADDREF(*aResult = mCurrentDownloads); Someone might think that the way to add a download is to QI this to nsIMutableArray and add a download - and it wouldn't even be an nsDownload object, which would be bad. Why not stick with a hashtable?
Assignee | ||
Comment 2•17 years ago
|
||
OK, this addresses my first pass of comments. It is untested and is just the backend code. I'll try to address any comments, so please make them. Neil - while it's true that someone *could* QueryInterface that, I'm returning an nsIArray for a reason (as in, it's not supposed to be mutable). So, if they want to shoot themselves in the foot, I'd rather let them than to hold their hand through the whole process. If you'd like, I can add more to the idl about not doing that, but I think the return type of nsIArray communicates that.
Attachment #264342 -
Attachment is obsolete: true
Comment 3•17 years ago
|
||
I agree that exposing an array which could be QIed to nsIMutableArray is sub-optimal, and confusing. Now, if there's no good reason to expose the array itself, I would rather expose it as a simple enumerator.
Assignee | ||
Comment 4•17 years ago
|
||
More work - fuller patch. Mostly posting to get help with my unit tests.
Attachment #264410 -
Attachment is obsolete: true
Assignee | ||
Comment 5•17 years ago
|
||
Attachment #264516 -
Attachment is obsolete: true
Assignee | ||
Comment 6•17 years ago
|
||
Attachment #264517 -
Attachment is obsolete: true
Assignee | ||
Comment 7•17 years ago
|
||
So, the image url, which is a parameter in nsIDownloadManager::AddDownload is never used. The only place the function is called is here: http://mxr.mozilla.org/seamonkey/source/toolkit/components/downloads/src/nsDownloadProxy.h#74 Note that that just passes EmptyString() along. Does anyone disagree with removing it?
Comment 8•17 years ago
|
||
Er? http://lxr.mozilla.org/seamonkey/source/toolkit/mozapps/downloads/content/downloads.js#651
Assignee | ||
Comment 9•17 years ago
|
||
Whoops. mxr doesn't work so well across languages :) Thanks for pointing that out!
Assignee | ||
Comment 10•17 years ago
|
||
Alright. This works in my testing quite nicely. The diff isn't very pretty due to the vast number of changes in nsDownloadManager.cpp, so whoever reviews this might want to apply it, then look at that file as it is. The tests don't quite work as they should yet, but I need to talk to people who are good with the testing framework to iron that out.
Attachment #264522 -
Attachment is obsolete: true
Comment 11•17 years ago
|
||
Forgive my ignorance of this newfangled mozStorage stuff Will extensions be able to: 1) Edit/Change existing parameters in the downloads database for specific downloads (i.e. it is not locked) 2) Dynamically add a new parameter for a specific download in the database (without confusing the back or front end - i.e. they will ignore or not even see these new parameters) Also, any idea on the performance of this compared to the current rdf implementation - specifically wrt large download histories?
Assignee | ||
Comment 12•17 years ago
|
||
(In reply to comment #11) > 1) Edit/Change existing parameters in the downloads database for specific > downloads (i.e. it is not locked) Sure, but they should be careful. > 2) Dynamically add a new parameter for a specific download in the database > (without confusing the back or front end - i.e. they will ignore or not even > see these new parameters) Sure, but I wouldn't recommend it. If we ever change anything internally, it could really screw things up. > Also, any idea on the performance of this compared to the current rdf > implementation - specifically wrt large download histories? It shouldn't be an issue, or at least not nearly as much of an issue as before.
Assignee | ||
Comment 13•17 years ago
|
||
Ok, this includes migration code. yey. The tests aren't done, but they are there for you to look at if you feel so incline. I'm not exactly happy with the migration code, but at the time I could think of no better way. Thoughts about it will be most appreciated.
Attachment #264679 -
Attachment is obsolete: true
Attachment #264842 -
Flags: review?(mconnor)
Attachment #264842 -
Flags: review?(cbiesinger)
Comment 14•17 years ago
|
||
uriloader/base/nsIDownload.idl + * The state of the download. + */ + readonly attribute short state; You should mention where to find the possible values for the state. It's somewhat unfortunate, though, that this means you have to reference a toolkit file in uriloader... Why is this not unsigned? Also, this patch will break Camino and SeaMonkey. It would be nice if you added those functions there (just returning an error). +++ toolkit/components/downloads/public/nsIDownloadManager.idl 15 May 2007 06:44:55 -0000 + * @throws NS_ERROR_FAILURE if the download is not in-progress. Or if no download with that id exists at all? (I kind of think that that functionality should just be on the nsIDownload object instead, but you don't have to change that for this patch :-) ) + * Stops the download with the specified ID if it's current in progress. This "currently" Also, timeless points out that this new comment is somewhat misleading, because it makes it non-obvious how the pause function differs from the cancel one - conceivably, a stopped download can be resumed again. I'd just stay with "Cancels the download ..." :) + * Removes the download with the specified id if it's not current in progress. here too. (it used to be currently, why did you change it?) + * Opens the Download Manager front end, making the specified download + * selected. Hm, wouldn't "selecting the specified download" sound better? What should I pass if I don't want to select any download? * An enumeration of active downloads. Please mention the type of the objects in the enumeration (nsIDownload) +++ toolkit/components/downloads/src/nsDownloadManager.cpp 15 May 2007 06:44:55 -0000 +static const nsInt64 sUpdateInterval((PRUint32)(400 * PR_USEC_PER_MSEC)); Since you touched this line :) Use PRInt64 instead of nsInt64, and remove the cast. Also - it should keep its g prefix, it's still a global. + nsresult result = CancelDownload(id); + // We want to try the rest of them because they should be canceled if they + // can be canceled. + if (NS_FAILED(result)) rv = result; + } So, this code worries me a bit. If you get a case where CancelDownload fails and leaves the download in the array, you've got an infinite loop. Why not make the loop look more like this: for (PRInt32 i = mCurrentDownloads.Count() - 1; i >= 0; --i) + printf("*** Added download '%s'\n", NS_ConvertUTF16toUTF8(name).get()); Remove that before checking this patch in + // lock will be released once we return What lock is this referring to? In nsDownloadManager::GetRetentionBehavior(), it would be nice to keep the comment about what 0 means. I'd have added inline setters for the various members of nsDownload, instead of accessing the variables directly. Hrm... you sometimes return an error from AddDownload and don't add the download to the DB or mCurrentDownloads, but still add it to the nsXPIProgressListener... Why did you move the dl-start notification elsewhere? + return CallQueryInterface(dl.get(), aDownload); No point to call QueryInterface, just do: NS_ADDREF(*aDownload = dl); return NS_OK; + nsRefPtr<nsDownload> itm = FindDownload(aID); Why does FindDownload return an addrefed download, instead of a non-addrefed raw pointer? If you change it there, make this a raw pointer too. + if (!itm) { + *aDownloadItem = nsnull; + + return NS_ERROR_FAILURE; remove that empty line + NS_ADDREF(*aDownloadItem = itm); - *aDownloadItem = nsnull; return NS_OK; That one too +nsDownloadManager::FindDownload(PRUint32 aID) +{ + // we shouldn't ever have many downloads, so we can loop over them + for (PRInt32 i = mCurrentDownloads.Count() - 1; i >= 0; --i) { Hm, why are you iterating backwards here? - // Take a strong reference to the download object as we may remove it from - // mCurrDownloads in DownloadEnded. That comment should stay, except for the reference to DownloadEnded + dl->GetCancelable(getter_AddRefs(cancelable)); ... + if (dl->mTempFile) { Why are you sometimes using the getter and sometimes using the variable directly? + // This have to be done in this exact order to not mess up our invariants What are those invariants? It would be nice to document them somewhere. + NS_ENSURE_TRUE(RemoveDownloadFromCurrent(dl), NS_ERROR_FAILURE); Hm, there is no case where this can fail here, is there? - // RemoveDownload is for downloads not currently in progress. Having it - // cancel in-progress downloads would make things complicated, so just return. Why remove that comment? - // 1). Downloads that can be cleaned up include those that are finished, - // failed or canceled. OK, I wouldn't have removed that one either, but I guess it's pretty obvious. + rv = stmt->GetInt32(0, aResult); I'd rather you didn't return values other than 0 and 1 in a boolean... mListener = aListener; + return NS_OK; You like those empty lines, don't you? :) nsDownloadManager::GetListener(nsIDownloadProgressListener** aListener) { - *aListener = mListener; - NS_IF_ADDREF(*aListener); - return NS_OK; -} + NS_ENSURE_ARG_POINTER(aListener); No point in that check really... +nsDownloadManager::GetDatasource(mozIStorageConnection **aDBConn) +{ + NS_ENSURE_ARG_POINTER(aDBConn); Neither here. Is GetDatasource a good name for this? Perhaps DBConnection might be better. +inline +PRBool +nsDownloadManager::RemoveDownloadFromCurrent(nsDownload *aDownload) Does this actually get inlined if you do it like this? While you're touching Observe, wanna change the nsCRT::strcmp calls to plain strcmp? +nsDownloadManager::ConfirmCancelDownloads(PRInt32 aCount, ... + { Why the block? No point in settinbg mDownloadManager to null in ~nsXPIProgressListener nsXPIProgressListener::OnStateChange(PRUint32 aIndex, PRInt16 aState, PRInt32 aValue) Shouldn't you call dl->UpdateDB where the previous code called AssertProgressInfoForDownload? Actually a general question, not limited to this function. - if (LL_IS_ZERO(mCurrBytes)) + if (LL_IS_ZERO(mCurrBytes)) { Since you're touching this, make it: if (mCurrBytes == 0) { Why did you make mPercentComplete a PRInt16? in nsDownload::OnStatusChange: + nsresult rv = UpdateDB(); + NS_ENSURE_SUCCESS(rv, rv); I don't think failure from UpdateDB should prevent the other code here from being run... + nsCOMPtr<nsIDownload> kungFuDeathGrip; + CallQueryInterface(this, NS_STATIC_CAST(nsIDownload**, + getter_AddRefs(kungFuDeathGrip))); nsRefPtr<nsDownload> kungFuDeathGrip(this); Much nicer, isn't it? :) But why did the old code not need this? + NS_ENSURE_TRUE(mDownloadManager->RemoveDownloadFromCurrent(this), + NS_ERROR_FAILURE); Don't think you should return here either. (Both OnStateChange and OnStatusChange) In OnStateChange, I don't think that lack of a pref service should be fatal. - if (mMaxBytes == -1) + if (mMaxBytes == PR_UINT32_MAX) Surely you mean LL_MAXUINT? + nsCOMPtr<nsIAlertsService> alerts = + do_GetService("@mozilla.org/alerts-service;1", &rv); + NS_ENSURE_SUCCESS(rv, rv); Again, this shouldn't be fatal. (the NS_ENSURE_SUCCESS line is also indented too much) +++ toolkit/components/downloads/src/nsDownloadProxy.h 15 May 2007 06:44:55 -0000 I'd make that file not implement nsIDownload anymore... -- BTW, is it a good idea to notify people about download state changes while you're in the middle of updating member variables? (e.g. in nsDownload::OnStateChange) Done with the C++ parts, I'll review the UI a bit later.
Assignee | ||
Comment 15•17 years ago
|
||
(In reply to comment #14) > You should mention where to find the possible values for the state. It's > somewhat unfortunate, though, that this means you have to reference a > toolkit file in uriloader... done > Why is this not unsigned? Because nsIDownloadManager and nsIXPInstallManagerUI use PRInt16's and not PRUInt16's. I'm just following the idl's in this case (and nsIDownloadManager::DOWNLOAD_NOTSTARTED = -1) > Also, this patch will break Camino and SeaMonkey. It would be nice if you > added those functions there (just returning an error). done (attaching patch of just those files in a bit) > + * @throws NS_ERROR_FAILURE if the download is not in-progress. > > Or if no download with that id exists at all? I would argue that that is already implied by being 'not in-progress' > + * Stops the download with the specified ID if it's current in progress. > Also, timeless points out that this new comment is somewhat misleading, > because it makes it non-obvious how the pause function differs from the > cancel one - conceivably, a stopped download can be resumed again. I had renamed the method, then reverted that but forgot to update the description. Fixed. > + * Removes the download with the specified id if it's not current in > progress. > here too. (it used to be currently, why did you change it?) I don't know...Fixed. > + * Opens the Download Manager front end, making the specified download > + * selected. > Hm, wouldn't "selecting the specified download" sound better? Yup. Fixed. > What should I pass if I don't want to select any download? We fail. Added documentation. I don't like this behavior at all, but it's what we did in the past, so I will file a follow-up bug and fix that once this lands. > * An enumeration of active downloads. > Please mention the type of the objects in the enumeration (nsIDownload) Done. > +static const nsInt64 sUpdateInterval((PRUint32)(400 * PR_USEC_PER_MSEC)); > Since you touched this line :) Use PRInt64 instead of nsInt64, and remove > the cast. Also - it should keep its g prefix, it's still a global. Fixed. > + nsresult result = CancelDownload(id); > + // We want to try the rest of them because they should be canceled if they > + // can be canceled. > + if (NS_FAILED(result)) rv = result; > + } > So, this code worries me a bit. If you get a case where CancelDownload > fails and leaves the download in the array, you've got an infinite loop. Wow, that *is* scary. Fixed. > + // lock will be released once we return > What lock is this referring to? Clarified. > In nsDownloadManager::GetRetentionBehavior(), it would be nice to keep the > comment about what 0 means. Fixed. > Hrm... you sometimes return an error from AddDownload and don't add the > download to the DB or mCurrentDownloads, but still add it to > the nsXPIProgressListener... Whoops. Fixed the ordering of the code to have anything that can error (just the database stuff) to happen before. > Why did you move the dl-start notification elsewhere? This is something that came up while testing. If something was listening for "dl-start", they would get the nsIDownload object, but if you checked the state of the download, it would be nsIDownloadManager::DOWNLOAD_NOTSTARTED. So, I only dispatch that when the download has actually started now. > + return CallQueryInterface(dl.get(), aDownload); > No point to call QueryInterface, just do... Fixed > + nsRefPtr<nsDownload> itm = FindDownload(aID); > Why does FindDownload return an addrefed download, instead of a > non-addrefed raw pointer? If you change it there, make this a raw pointer > too. Actually, no good reason. I've updated the file accordingly. > + // we shouldn't ever have many downloads, so we can loop over them > + for (PRInt32 i = mCurrentDownloads.Count() - 1; i >= 0; --i) { > Hm, why are you iterating backwards here? To avoid a function call on every loop to Count. > - // Take a strong reference to the download object as we may remove it from > - // mCurrDownloads in DownloadEnded. > That comment should stay, except for the reference to DownloadEnded I ended up adding one in the process of fixing FindDowload. Fixed. > + dl->GetCancelable(getter_AddRefs(cancelable)); > ... > + if (dl->mTempFile) { > Why are you sometimes using the getter and sometimes using the variable > directly? Mostly because I missed some. I cleaned up any others I found. > + // This have to be done in this exact order to not mess up our invariants > What are those invariants? It would be nice to document them somewhere. So, I'm not really sure where I should document it, or if the word invariant is quite right in my application, but it basically comes down to this: First we remove the download from current downloads. We then set the state of the download, which calls OnDownloadStateChange on the download listener. This passes an nsIDownload along with it, so we have to make sure that if someone were to then try and use any functions (remove, cancel, pause, resume), that the download would be out of the current ones. Once we update the state, we can dispatch "dl-cancel", and the download state will actually be nsIDownloadManager::DOWNLOAD_CANCELED as one would expect. This all came out while testing it and getting results that were unexpected, so I fixed it to work in a way that makes sense. > + NS_ENSURE_TRUE(RemoveDownloadFromCurrent(dl), NS_ERROR_FAILURE); > Hm, there is no case where this can fail here, is there? After hunting down all the function calls, it will only fail if the object isn't in the array, which I check for, so no I do not have to check that. > - // RemoveDownload is for downloads not currently in progress. Having it > - // cancel in-progress downloads would make things complicated, so just > return. > Why remove that comment? Because the IDL file explicitly states what the expected behavior of the function is. > + rv = stmt->GetInt32(0, aResult); > I'd rather you didn't return values other than 0 and 1 in a boolean... Fair enough, fixed. > nsDownloadManager::GetListener(nsIDownloadProgressListener** aListener) > { > + NS_ENSURE_ARG_POINTER(aListener); > No point in that check really... Fixed. > +nsDownloadManager::GetDatasource(mozIStorageConnection **aDBConn) > +{ > + NS_ENSURE_ARG_POINTER(aDBConn); Fixed. > Neither here. Is GetDatasource a good name for this? Perhaps DBConnection > might be better. I agree. Updated accordingly. > +inline > +PRBool > +nsDownloadManager::RemoveDownloadFromCurrent(nsDownload *aDownload) > Does this actually get inlined if you do it like this? I'm not so sure. I've moved it to the header file where I'm fairly certain that it is. > While you're touching Observe, wanna change the nsCRT::strcmp calls to > plain strcmp? Sure. > +nsDownloadManager::ConfirmCancelDownloads(PRInt32 aCount, > ... > + { > Why the block? I uh....don't know. Fixed. > No point in setting mDownloadManager to null in ~nsXPIProgressListener I don't think I touched that, but I can fix that. > Shouldn't you call dl->UpdateDB where the previous code called > AssertProgressInfoForDownload? Actually a general question, not limited to > this function. Generally, yes. I only need to do it whenever we set some data, so I think (thought) I got them all. I did miss that one at least. I'll look for others. > - if (LL_IS_ZERO(mCurrBytes)) > + if (LL_IS_ZERO(mCurrBytes)) { > Since you're touching this, make it: > if (mCurrBytes == 0) { Fixed. > Why did you make mPercentComplete a PRInt16? Er...mostly because that's what I thought the IDL had, but looking again it doesn't. I'll revert that back. > in nsDownload::OnStatusChange: > + nsresult rv = UpdateDB(); > + NS_ENSURE_SUCCESS(rv, rv); > I don't think failure from UpdateDB should prevent the other code here > from being run... Yeah, fair enough. Fixed. > + nsCOMPtr<nsIDownload> kungFuDeathGrip; > + CallQueryInterface(this, NS_STATIC_CAST(nsIDownload**, > + getter_AddRefs(kungFuDeathGrip))); > > nsRefPtr<nsDownload> kungFuDeathGrip(this); > Much nicer, isn't it? :) Yes, much! > But why did the old code not need this? Because when it is removed from mCurrentDownloads, it releases it. However, with moving that call to the end (which is OK for me to do), I can get away with not doing it. Fixed. > In OnStateChange, I don't think that lack of a pref service should be > fatal. Good point. Fixed - all calls check before using |if (pref)...| > - if (mMaxBytes == -1) > + if (mMaxBytes == PR_UINT32_MAX) > Surely you mean LL_MAXUINT? Surely. > + nsCOMPtr<nsIAlertsService> alerts = > + do_GetService("@mozilla.org/alerts-service;1", &rv); > + NS_ENSURE_SUCCESS(rv, rv); > Again, this shouldn't be fatal. (the NS_ENSURE_SUCCESS line is also > indented too much) Yeah, fixed. > +++ toolkit/components/downloads/src/nsDownloadProxy.h 15 May 2007 06:44:55 > -0000 > I'd make that file not implement nsIDownload anymore... Done. I'm not really sure why it was included in the first place, and the bug associated with it doesn't really explain it either. > BTW, is it a good idea to notify people about download state changes > while you're in the middle of updating member variables? (e.g. in > nsDownload::OnStateChange) I found one instance where I wasn't doing that wisely (the function you mentioned actually). Fixed.
Status: NEW → ASSIGNED
Assignee | ||
Comment 16•17 years ago
|
||
Attachment #264842 -
Attachment is obsolete: true
Attachment #264842 -
Flags: review?(mconnor)
Attachment #264842 -
Flags: review?(cbiesinger)
Assignee | ||
Comment 17•17 years ago
|
||
Attachment #264976 -
Flags: review?(mconnor)
Assignee | ||
Comment 18•17 years ago
|
||
Attachment #264977 -
Flags: review?(mconnor)
Attachment #264977 -
Flags: review?(cbiesinger)
Comment 19•17 years ago
|
||
Comment on attachment 264976 [details] [diff] [review] v1.0 c++ backend; idl changes >Index: uriloader/base/nsIDownload.idl >+ readonly attribute unsigned long id; >+ readonly attribute short state; Maybe it's just me (others should note any disagreement), but I don't have the conversions between C++-style IDL types and PR* types memorized. Could you expand these into PR* types while you're here? You could probably even do this for interfaces you're not otherwise changing (e.g. the one containing download states), because I hope (would need to test) that there's no ABI change from doing a "same-type" switch (e.g. long to PRInt32) of a parameter or return value type.
Comment 20•17 years ago
|
||
Aren't PR-types in XPIDL supported just for backwards-compatibility (well, except PRTime I guess). I cannot see why we would want to expose PRTypes to say, js consumers. You don't need to memorize anything, just look at the header ;)
Comment 21•17 years ago
|
||
http://lxr.mozilla.org/seamonkey/source/xpcom/base/nsrootidl.idl#61 for reference
Comment 22•17 years ago
|
||
Also <http://www.mozilla.org/scriptable/xpidl/idl-authors-guide/rules.html>, but I still have to look one or the other up every time. Also, it's precisely the C++ case that concerns me. A coder in JS-land can just use the simple rule that *Int* is an integer, *Uint* is an unsigned integer, etc. and otherwise ignore PR* types. A coder in C++-land has no such rule for correctly mapping onto concrete types and must fall back on the header or some other documentation. Also, if the PR type are only for backwards compat, there's an awfully large number of new interfaces still choose to use them.
Assignee | ||
Comment 23•17 years ago
|
||
This brought up a few minor changes to the v1.0 c++ backend patch, but I won't bother attaching that unless mconnor wants it. The code isn't terribly different - just fixed a few issues that came up while testing.
Attachment #265058 -
Flags: review?(mconnor)
Comment 24•17 years ago
|
||
>> + for (PRInt32 i = mCurrentDownloads.Count() - 1; i >= 0; --i) { >> Hm, why are you iterating backwards here? > To avoid a function call on every loop to Count. Well, you could have done: for (PRUint32 i = 0, e = mCurrentDownloads.Count(); i < e; ++i) > So, I'm not really sure where I should document it, or if the word invariant is > quite right in my application If in doubt, try the place where you mention that they exist. Really, if someone will modify this code in the future, how are they to know what this code tries to ensure? > We then set the state of > the download, which calls OnDownloadStateChange on the download listener. This > passes an nsIDownload along with it, so we have to make sure that if someone > were to then try and use any functions (remove, cancel, pause, resume), that > the download would be out of the current ones. Once we update the state, we > can dispatch "dl-cancel", and the download state will actually be > nsIDownloadManager::DOWNLOAD_CANCELED as one would expect. Will those methods really work on a cancelled download? Also: You asked me about whether to add a downloadStateChanged method at one point. I didn't know about these observer service notifications (dl-start, dl-cancel, etc). Given those, is it really necessary to add another mechanism for those notifications? (I think there's some value in keeping nsIDownloadProgressListener in sync with nsIWebProgressListener) > Maybe it's just me (others should note any disagreement), but I don't have the > conversions between C++-style IDL types and PR* types memorized. Could you > expand these into PR* types while you're here? Hm, I think they look ugly...
Comment 25•17 years ago
|
||
as for the UpdateDB calls where there used to be an AssertProgressInfoFor: It seems that CancelAllDownloads is missing them. GetDownload and GetDBConnection have a useless NS_ENSURE_ARG_POINTER too, I must've missed them before are you no longer checking downloadCount in the observer for offline-requested? + nsAutoString countString(aCount); Does that actually do the right thing? + PRTime delta = now - mLastUpdate; I always considered PRTime to be just meant to store µsec since the epoch, not also deltas... in OnStatusChange: nsCOMPtr<nsIDownload> kungFuDeathGrip; CallQueryInterface(this, NS_STATIC_CAST(nsIDownload**, getter_AddRefs(kungFuDeathGrip))); mDownloadManager->RemoveDownloadFromCurrent(this); see nsRefPtr<> sugegestion in an earlier comment You now call UpdateDB both in nsDownloadManager::PauseResumeDownload and in nsDownload::PauseResume, I'd remove it from the former.
Comment 26•17 years ago
|
||
Comment on attachment 264977 [details] [diff] [review] v1.0 frontend changes nsDownloadProgressListener.js: var aDownloadID = aDownload.targetFile.path; var download = this.doc.getElementById(aDownloadID); HTML neither allows slashes nor backslashes in an ID. Also, non-arguments shouldn't start with "a" downloads.js: const nsILocalFile = Components.Constructor("@mozilla.org/file/local;1", "nsILocalFile", "initWithPath"); I don't like naming non-interfaces nsI*. Please rename this to nsLocalFile. -- Do Remove/Cleanup work correctly? I.e., do they update the UI to remove the removed downloads? (because I don't see how this can work...) [this wasn't a full review]
Comment 27•17 years ago
|
||
(In reply to comment #25) > + PRTime delta = now - mLastUpdate; > > I always considered PRTime to be just meant to store µsec since the epoch, not > also deltas... Yeah; use PRIntervalTime for deltas. It has the advantage of not getting confused if your system clock changes (eg is reset by you or updated by NTP).
Updated•17 years ago
|
Summary: Convert rdf backend to mozStorage → Convert Download Manager's RDF backend to mozStorage
Assignee | ||
Comment 28•17 years ago
|
||
(In reply to comment #24) > If in doubt, try the place where you mention that they exist. Really, if > someone will modify this code in the future, how are they to know what this > code tries to ensure? Fixed. > Will those methods really work on a cancelled download? Uh, no - I think I was talking about more in general with the observers/listeners. > Also: You asked me about whether to add a downloadStateChanged method at one > point. I didn't know about these observer service notifications (dl-start, > dl-cancel, etc). Given those, is it really necessary to add another mechanism > for those notifications? (I think there's some value in keeping > nsIDownloadProgressListener in sync with nsIWebProgressListener) As per discussion on irc, we'll keep this but have it send the old state and the download as opposed to the new state (since the new state can be found on the download anyway). (In reply to comment #25) > as for the UpdateDB calls where there used to be an AssertProgressInfoFor: > It seems that CancelAllDownloads is missing them. Yes, it is, but it calls CancelDownload on each one, which has that on its last line. > GetDownload and GetDBConnection have a useless NS_ENSURE_ARG_POINTER too, I > must've missed them before Fixed. > are you no longer checking downloadCount in the observer for offline-requested? Uh, whoops :) Fixed. > + nsAutoString countString(aCount); > Does that actually do the right thing? I can't find any code to prove that it does, but I thought it did. I'm going to revert that back to the old way though. > I always considered PRTime to be just meant to store µsec since the epoch, not > also deltas... Updated to dmose's suggestion. > in OnStatusChange: > nsCOMPtr<nsIDownload> kungFuDeathGrip; > CallQueryInterface(this, NS_STATIC_CAST(nsIDownload**, > getter_AddRefs(kungFuDeathGrip))); > mDownloadManager->RemoveDownloadFromCurrent(this); > > see nsRefPtr<> sugegestion in an earlier comment Fixed. > You now call UpdateDB both in nsDownloadManager::PauseResumeDownload and in > nsDownload::PauseResume, I'd remove it from the former. Hmm, that's extra overhead I don't want. Fixed.
Assignee | ||
Comment 29•17 years ago
|
||
Attachment #264976 -
Attachment is obsolete: true
Attachment #265132 -
Flags: review?(cbiesinger)
Attachment #264976 -
Flags: review?(mconnor)
Assignee | ||
Comment 30•17 years ago
|
||
Updated accordingly based on changes in backend/api
Attachment #265058 -
Attachment is obsolete: true
Attachment #265058 -
Flags: review?(mconnor)
Assignee | ||
Comment 31•17 years ago
|
||
-N is helpful :/
Attachment #265133 -
Attachment is obsolete: true
Attachment #265134 -
Flags: review?(mconnor)
Assignee | ||
Comment 32•17 years ago
|
||
Addresses biesi's comments, as well as fixing at least one other issue that I found.
Attachment #264977 -
Attachment is obsolete: true
Attachment #265150 -
Flags: review?(mconnor)
Attachment #265150 -
Flags: review?(cbiesinger)
Attachment #264977 -
Flags: review?(mconnor)
Attachment #264977 -
Flags: review?(cbiesinger)
Comment 33•17 years ago
|
||
Comment on attachment 265150 [details] [diff] [review] v1.1 frontend changes I'll let mconnor review this
Attachment #265150 -
Flags: review?(cbiesinger)
Comment 34•17 years ago
|
||
Comment on attachment 265134 [details] [diff] [review] v1.1.1 tests +++ toolkit/components/downloads/test/Makefile.in 17 May 2007 18:39:04 -0000 +# Contributor(s): +# Shawn Wilsher <sdwilsh@mozilla.com> (Original Author) +# Alternatively, the contents of this file may be used under the terms of I'd put an empty line between your name and the rest of the license. Also, most people don't indent the names this much.. +++ toolkit/components/downloads/test/unit/test_download_manager.js 17 May 2007 18:39:04 -0000 + Math.round(Date.now() / 1000), null, persist); are you sure that you don't mean * 1000? +function test_addDownload_cancel() +{ + var dl = addDownload(); + + dm.cancelDownload(dl.id); +} Perhaps test that the state is CANCELLED after this?
Attachment #265134 -
Flags: review+
Comment 35•17 years ago
|
||
Comment on attachment 265132 [details] [diff] [review] v1.1 c++ backend; idl changes in UpdateDB, you are storing src where you should store target.
Attachment #265132 -
Flags: review?(cbiesinger) → review+
Comment 36•17 years ago
|
||
Comment on attachment 265150 [details] [diff] [review] v1.1 frontend changes >Index: toolkit/mozapps/downloads/content/DownloadProgressListener.js >=================================================================== >RCS file: /cvsroot/mozilla/toolkit/mozapps/downloads/content/DownloadProgressListener.js,v >retrieving revision 1.11 >diff -u -8 -p -r1.11 DownloadProgressListener.js >--- toolkit/mozapps/downloads/content/DownloadProgressListener.js 6 Feb 2006 00:34:29 -0000 1.11 >+++ toolkit/mozapps/downloads/content/DownloadProgressListener.js 17 May 2007 21:41:04 -0000 >@@ -59,21 +59,29 @@ function DownloadProgressListener (aDocu > > DownloadProgressListener.prototype = > { > rateChanges: 0, > rateChangeLimit: 0, > priorRate: 0, > lastUpdate: -500, > doc: null, >+ onDownloadStateChange: function(aState, aDownload) >+ { what we should do is something like: onDownloadStateChange: function dlProgressListener_onDownloadStateChange(aState, aDownload) so JS debugging sucks less, but file a folloup on cleaning up this file if its sticking around, rather than fixing now. >+var gDownloadManager = Components.classes[kDlmgrContractID] >+ .getService(nsIDownloadManager);; extra semicolon >+function createDownloadItem(aID, aFile, aImage, aTarget, aURI, aState, >+ aAnimated, aStatus, aProgress) >+{ >+ var dl = document.createElement("download"); >+ dl.setAttribute("id", "dl" + aID); >+ dl.setAttribute("dlid", aID); >+ dl.setAttribute("image", aImage); >+ dl.setAttribute("file", aFile); >+ dl.setAttribute("target", aTarget); >+ dl.setAttribute("uri", aURI); >+ dl.setAttribute("state", aState); >+ dl.setAttribute("aimated", aAnimated); animated this seems to have a really huge signature, I guess its just a helper method. Might make more sense to pass in an array, but probably not worth it for a couple of callers >+ var dl = document.getElementById("dl" + aDownload.id); you repeat this piece a lot, I wonder if its worth factoring out into getDownloadId(download) for readability and avoiding copying this pattern all over the place >+ // Adding to the UI >+ var uri = Components.classes["@mozilla.org/network/util;1"] >+ .getService(Components.interfaces.nsIIOService) >+ .newFileURI(dl.targetFile); >+ var img = "moz-icon://" + uri.spec + "?size=32"; >+ var itm = createDownloadItem(dl.id, dl.targetFile.path, img, >+ dl.displayName, dl.source, dl.state, "", >+ dl.percentComplete); >+ gDownloadsView.insertBefore(itm, gDownloadsView.firstChild); >+ >+ // switch view to it >+ gDownloadsView.selectedIndex = 0; so, I'm kinda wondering if we want to just not do this, changing the focus around is kinda bad probably out of scope though, file a followup >+ // Update UI >+ for (var i = gDownloadsView.children.length - 1; i >= 0; --i) { >+ var elm = gDownloadsView.children[i]; >+ var state = elm.getAttribute("state"); >+ >+ if (state != nsIDownloadManager.DOWNLOAD_NOTSTARTED && >+ state != nsIDownloadManager.DOWNLOAD_DOWNLOADING && >+ state != nsIDownloadManager.DOWNLOAD_PAUSED && >+ state != nsIXPInstallManagerUI.INSTALL_DOWNLOADING && >+ state != nsIXPInstallManagerUI.INSTALL_INSTALLING) >+ gDownloadsView.removeChild(gDownloadsView.children[i]); >+ } FYI only: This type of thing is generally faster as a switch, though it seems about the same for ints. >- <script type="application/x-javascript" src="chrome://mozapps/content/downloads/DownloadProgressListener.js"/> >- <script type="application/x-javascript" src="chrome://mozapps/content/downloads/downloads.js"/> >- <script type="application/x-javascript" src="chrome://global/content/contentAreaUtils.js"/> >- <script type="application/x-javascript" src="chrome://global/content/nsDragAndDrop.js"/> >- <script type="application/x-javascript" src="chrome://global/content/globalOverlay.js"/> >+ <script type="application/javascript" src="chrome://mozapps/content/downloads/DownloadProgressListener.js"/> >+ <script type="application/javascript" src="chrome://mozapps/content/downloads/downloads.js"/> >+ <script type="application/javascript" src="chrome://global/content/contentAreaUtils.js"/> >+ <script type="application/javascript" src="chrome://global/content/nsDragAndDrop.js"/> >+ <script type="application/javascript" src="chrome://global/content/globalOverlay.js"/> Now that's nitpicky. I like it. r=me
Attachment #265150 -
Flags: review?(mconnor) → review+
Comment 37•17 years ago
|
||
> >- <script type="application/x-javascript" src="chrome://global/content/globalOverlay.js"/> > >+ <script type="application/javascript" src="chrome://global/content/globalOverlay.js"/> > > Now that's nitpicky. I like it. > Is application/javascript preferred over application/x-javascript now? Why? The x-javascript version is the de-facto standard in moz code - see http://lxr.mozilla.org/mozilla/source/content/xul/document/src/nsXULContentSink.cpp#1018
Comment 38•17 years ago
|
||
Comment on attachment 265134 [details] [diff] [review] v1.1.1 tests Index: toolkit/components/downloads/test/unit/test_download_manager.js missing MPL boilerplate + biesi's comments about indenting in the boilerplate +const nsIDownloadManager = Ci.nsIDownloadManager; +const dm = Components.classes["@mozilla.org/download-manager;1"] + .getService(nsIDownloadManager); nit: why not use Cc here? r=me, good to have some tests here...
Attachment #265134 -
Flags: review?(mconnor) → review+
Comment 39•17 years ago
|
||
(In reply to comment #37) > Is application/javascript preferred over application/x-javascript now? Why? The > x-javascript version is the de-facto standard in moz code - see application/javascript has been registered recentlyish, http://www.rfc-editor.org/rfc/rfc4329.txt
Assignee | ||
Comment 40•17 years ago
|
||
Addresses comments.
Attachment #265134 -
Attachment is obsolete: true
Assignee | ||
Comment 41•17 years ago
|
||
Addresses comments
Attachment #265150 -
Attachment is obsolete: true
Assignee | ||
Comment 42•17 years ago
|
||
Addresses Comments
Attachment #265132 -
Attachment is obsolete: true
Assignee | ||
Comment 43•17 years ago
|
||
Checking in toolkit/components/downloads/Makefile.in; new revision: 1.4; previous revision: 1.3 Checking in toolkit/components/downloads/test/Makefile.in; initial revision: 1.1 Checking in toolkit/components/downloads/test/unit/test_download_manager.js; initial revision: 1.1 Checking in toolkit/components/downloads/test/unit/test_download_manager_migration.js; initial revision: 1.1 Checking in toolkit/components/downloads/test/unit/downloads.rdf; initial revision: 1.1 Checking in uriloader/base/nsIDownload.idl; new revision: 1.18; previous revision: 1.17 Checking in toolkit/components/downloads/public/nsIDownloadManager.idl; new revision: 1.9; previous revision: 1.8 Checking in toolkit/components/downloads/public/nsIDownloadProgressListener.idl; new revision: 1.5; previous revision: 1.4 Checking in toolkit/components/downloads/src/nsDownloadManager.h; new revision: 1.23; previous revision: 1.22 Checking in toolkit/components/downloads/src/nsDownloadManager.cpp; new revision: 1.73; previous revision: 1.72 Checking in toolkit/components/downloads/src/nsDownloadProxy.h; new revision: 1.18; previous revision: 1.17 Checking in toolkit/components/downloads/src/Makefile.in; new revision: 1.12; previous revision: 1.11 Checking in toolkit/components/build/Makefile.in; new revision: 1.48; previous revision: 1.47 Checking in toolkit/mozapps/downloads/content/DownloadProgressListener.js; new revision: 1.12; previous revision: 1.11 Checking in toolkit/mozapps/downloads/content/downloads.js; new revision: 1.56; previous revision: 1.55 Checking in toolkit/mozapps/downloads/content/downloads.xul; new revision: 1.22; previous revision: 1.21 Checking in embedding/browser/cocoa/src/nsDownloadListener.mm; new revision: 1.11; previous revision: 1.10 Checking in xpfe/components/download-manager/src/nsDownloadManager.cpp; new revision: 1.122; previous revision: 1.121 Checking in camino/src/download/nsDownloadListener.mm; new revision: 1.30; previous revision: 1.29
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•17 years ago
|
Target Milestone: --- → Firefox 3 alpha4
The fact that we now have downloads.sqlite, rather than downloads.rdf generated, in addition to my LXR search/Bonsai query, should be good enough to verify that we've switched over. Verified FIXED.
Status: RESOLVED → VERIFIED
Comment 45•17 years ago
|
||
I'm getting the docs done for this, and have a few questions, figured this is a good place to post them since the people cc'd on this bug probably know way more about this than I do... Questions: 1. What is a download type? There's only one value (DOWNLOAD_TYPE_DOWNLOAD), so I'm not clear on the purpose to this. 2. What are DOWNLOAD_BLOCKED and DOWNLOAD_SCANNING for? If I had to guess, I would assume these are states that extensions can use to block downloads or mark them as "being scanned for viruses," but I have no actual reason to say that other than it being what seems most likely to me. I may have other questions later, but that'll get me started. The freshly minted doc for nsIDownloadManager is here: http://developer.mozilla.org/en/docs/nsIDownloadManager
Assignee | ||
Comment 46•17 years ago
|
||
(In reply to comment #45) > 1. What is a download type? There's only one value (DOWNLOAD_TYPE_DOWNLOAD), > so I'm not clear on the purpose to this. I've been strongly considering filing a bug to remove that as it isn't really used. > 2. What are DOWNLOAD_BLOCKED and DOWNLOAD_SCANNING for? If I had to guess, I > would assume these are states that extensions can use to block downloads or > mark them as "being scanned for viruses," but I have no actual reason to say > that other than it being what seems most likely to me. These are really just states of a download. You cannot actually set any download to this state - it's just possible values for nsIDownload.state
Comment 47•17 years ago
|
||
So what do DOWNLOAD_BLOCKED and DOWNLOAD_SCANNING mean then? :)
Assignee | ||
Comment 48•17 years ago
|
||
Whoops, sorry. DOWNLOAD_BLOCKED can happen if a download is blocked by parental controls or if a download contains a virus that cannot be cleaned. DOWNLOAD_SCANNING is just a download that is being scanned for viruses.
Comment 49•17 years ago
|
||
Firefox has controls that cause these states to occur? I wasn't aware of there being a virus scanner in Firefox.
Assignee | ||
Comment 50•17 years ago
|
||
It just recently landed (Bug 103487)
Updated•16 years ago
|
Product: Firefox → Toolkit
Comment 53•12 years ago
|
||
Comment on attachment 265532 [details] [diff] [review] v1.2 backend; idl >+ nsCOMArray<nsDownload> mCurrentDownloads; nsCOMArray is only allowed to have interface pointers, not concrete types...
You need to log in
before you can comment on or make changes to this bug.
Description
•