We should do it otherwise the historical downloads will always come with unknown state.

The suggested way is likely a method in nsIDownloadHistory, something like onDownloadStateChanged may do. we may leak a bit into the Aurora timeframe if we do that early, like the first week.

This, in fact, does not affect performance.

I'm not sure (and I have not tested) what happens when one tries to annotate an unvisited uri with EXPIRE_WITH_HISTORY. I should probably check if the url was visited either way.

Comment on attachment 701731 [details] [diff] [review] patch Review of attachment 701731 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/downloads/content/allDownloadsViewOverlay.js @@ +272,5 @@ > function onSuccess(fileInfo) { > this._targetFileInfoFetched = true; > this._targetFileExists = true; > this._targetFileSize = fileInfo.size; > + this._metaData.fileSize = this._targetFileSize; are we sure _metaData is always valid at this point, since _refreshInternalState can set it to null? I think a null check is worth it... @@ +291,5 @@ > }.bind(this) > ); > }, > > + _getHistoryDownloadMetaData: function DES__getHistoryDownloadMetaData() { I wonder if we should change it to getDownloadMetaData and return the value from dataItem when available... so we kill getDownloadState and avoid things like this._dataItem.endTime || this._getHistoryDownloadMetaData().endTime; Though I guess this way it's clearer where the data comes from... @@ +304,5 @@ > + nsIDM.DOWNLOAD_FINISHED : nsIDM.DOWNLOAD_FAILED; > + this._metaData.fileSize = this._targetFileSize; > + } > + > + // This is actually the end-time, but it's the best we can get. I suppose you meant the start-time? @@ +327,5 @@ > + let metaData = this._getHistoryDownloadMetaData(); > + if ("state" in metaData) > + return metaData.state; > + > + return undefined; doesn't just return metaData.state return undefined if "state" is not in metaData? I suppose just return this._getHistoryDownloadMetaData().state; should do @@ +404,2 @@ > let date = this._dataItem && this._dataItem.endTime || > + this._getHistoryDownloadMetaData().endTime; fix indentation @@ +498,5 @@ > } > } > }, > > + _refreshInternalState: function DES__refreshInternalState() { I don't like much the name cause internal-state may be anything, but don't have better ideas atm. ::: browser/components/downloads/src/DownloadsCommon.jsm @@ +1002,5 @@ > this._notifyDownloadEvent("finish"); > } > + > + // XXXmano: this isn't the right place to set these annotation. We should > + // probably set it in places' nsIDownloadHistory implementation. remember to file the bug and add bug# here. The problem we'll have with that is that I'm not sure how hard is to encode/decode json in cpp, but I assume it's feasible (jsapi and nsIJSON). @@ +1006,5 @@ > + // probably set it in places' nsIDownloadHistory implementation. > + if (!this._isPrivate && !dataItem.inProgress) { > + let downloadMetaData = { state: dataItem.state, > + startItem: dataItem.startItem, > + endTime: dataItem.endTime }; future scope-creep: in future we should also add the target uri here and reduce all annos to one finally. just matter of storing a metadata on download start. What is startItem? maybe you meant startTime... Why do we need start time, is not that just the visit time that we already have? @@ +1012,5 @@ > + downloadMetaData.fileSize = dataItem.maxBytes; > + > + PlacesUtils.annotations.setPageAnnotation( > + NetUtil.newURI(dataItem.uri), "download/metaData", JSON.stringify(downloadMetaData), 0, > + PlacesUtils.annotations.EXPIRE_WITH_HISTORY); to answer your question, if you do this on a not-existing places uri it should throw. if you set it on a page that is in the database but has no visits it will just be expired asap. I suppose a try/catch(reportError) here may be useful to catch eventual misses.

Comment on attachment 701871 [details] [diff] [review] patch Review of attachment 701871 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/downloads/content/allDownloadsViewOverlay.js @@ +28,5 @@ > const nsIDM = Ci.nsIDownloadManager; > > const DESTINATION_FILE_URI_ANNO = "downloads/destinationFileURI"; > const DESTINATION_FILE_NAME_ANNO = "downloads/destinationFileName"; > +const DOWNLOAD_META_DATA_ANNO = "download/metaData"; please retain the same annotation space, even if it's related to a single download, also destinationFileURI and downloads/destinationFileName are... so let's just keep using the downloads (plural) namespace @@ +118,5 @@ > set dataItem(aValue) { > this._dataItem = aValue; > let shouldUpdate = false; > if (this._dataItem) { > + this._invalidateMetaDataAndTargetFilInfo(); typo (I assume copy/pasted over all the patch, please s/r: "FilInfo" instead of "FileInfo" @@ +291,5 @@ > }.bind(this) > ); > }, > > + _getDownloadMetaData: function DES__getDownloadMetaData() { nit: would be nice to document the available properties on the returned object in a javadoc @@ +355,5 @@ > } > > // This is a not-in-progress or history download. > let stateLabel = ""; > + switch (this._getDownloadMetaData().state) { this is firing a warning on view load Timestamp: 14/01/2013 21:09:14 Warning: ReferenceError: reference to undefined property this._getDownloadMetaData(...).state @@ +396,1 @@ > let [displayDate, fullDate] = DownloadUtils.getReadableDates(new Date(date)); nit: move "new Date()" to the previous assignment, so this looks even nicer ::: browser/components/downloads/src/DownloadsCommon.jsm @@ +1001,5 @@ > if (dataItem.done) { > this._notifyDownloadEvent("finish"); > } > + > + // XXX Bug 830415: this isn't the right place to set these annotation. s/XXX/TODO/

Comment on attachment 701960 [details] [diff] [review] patch [Approval Request Comment] Bug caused by (feature/regressing bug #): Downloads panel feature User impact if declined: incomplete ui Testing completed (on m-c, etc.): m-i (merge pending) Risk to taking this patch (and alternatives if risky): Limited to the feature String or UUID changes made by this patch: none

Comment on attachment 701960 [details] [diff] [review] patch I have a follow-up fixing some errors I saw in the console. will re-ask approval on a merged patch

onStateChange was throwing due to _targetFileInfoFetched being called again... regardless I think we should not throw in the middle of the onDownloadStateChanged notification otherwise all of the views go mad, so better to reportError there.

Actually, it's much more than that. I figured this morning we use this new annotation very poorly. follow-up patch coming in a little bit (also removing the file name annotation usage).

This should have a very positive impact on performance for newly created history downloads. I apologize for the somewhat large patch. The code is much more readable now though.

Comment on attachment 702257 [details] [diff] [review] address review comments given on irc, incorporate Marco's fixes Review of attachment 702257 [details] [diff] [review]: ----------------------------------------------------------------- these comments, plus fix remaining usage of _displayName as said on IRC ::: browser/components/downloads/content/allDownloadsViewOverlay.js @@ +281,5 @@ > * may be set. > * > * state - any download state defined in nsIDownloadManager. If this field > * is not set, the download state is unknown. > + * - endTime: the end time of the download. please update indentation of "state" property @@ +334,5 @@ > + this._extractFilePathAndNameFromFileURI(targetFileURI); > + } > + catch(ex) { > + this._metaData.displayName = > + this._placesNode.title || this.downloadURI; trailing spaces and maybe could be onelined @@ +507,5 @@ > > placesNodeTitleChanged: function DES_placesNodeTitleChanged() { > + // If there's a file path, we use the leaf name for the title. > + if (!this._dataItem && this.active && !this._getDownloadMetaData().filePath) { > + delete this._metaData; I thought we agreed to use this._metaData = null; @@ +733,5 @@ > + // the target file information. Thus the call to goUpdateDownloadCommands > + // in DPV_onSelect would result in best-guess enabled/disabled result. > + // That way we let the user perform command immediately. However, once > + // we have the target file information, we can update the commands > + // appropriately (_fetchTargetFileInfo() calls goUpdateDownloadCommands). move to javadoc

Comment on attachment 702334 [details] [diff] [review] combined patch for Aurora [Approval Request Comment] Bug caused by (feature/regressing bug #): New downloads view User impact if declined: performance issues. Testing completed (on m-c, etc.): there are no tests for this view at this point. Risk to taking this patch (and alternatives if risky): limited to the feature. String or UUID changes made by this patch: none.

https://hg.mozilla.org/mozilla-central/rev/43ae0a302e8c https://hg.mozilla.org/mozilla-central/rev/5714c31c6def

Verified as fixed on Windows 7, Ubuntu 12.04 and Mac OS X 10.8: Build ID: 20130326150557 Mozilla/5.0 (Windows NT 6.1; rv:20.0) Gecko/20100101 Firefox/20.0 Mozilla/5.0 (X11; Linux i686; rv:20.0) Gecko/20100101 Firefox/20.0 Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:20.0) Gecko/20100101 Firefox/20.0