This bug is to define and implement additional interaction with downloads shown in the Library Downloads section.

If I understand correctly, the first thing that we need to do is to make it so that clicking items displayed in the Downloads folder opens the download target file, if still present in the original save location. We'll probably need to change the label default action in the context menu too. Marco, I can probably take this bug in a few days from now, if no one else is available, if you give me a couple of pointers on the approach we could use.

we should first store the needed data in the db (bug 591289), so I'd prefer if that would be worked on first. Here we will probably need to handle the commands in places.js and the treeview controller

This bug needs a list of actions and context menus that should be added.

Might I suggest these for the context menu? Clicking twice on the file should open it, this is mandatory. Open folder Copy download URL Go to URL Delete from System Remove from Library list Rename. Personally I believe these should go both in the new download manager and the new download library window

Thanks for bug 564900! What about "Go to Download Page" that's in the current Downloads window's context menu before "Copy Download Link"? I hope Downloads in Library remembers this page (is it stored in Places as aReferrer?) as well as the download's own URL. Often you don't want to download the same URL again, instead you want to go to the page from where you downloaded it to see if there's a later version to download. If Places also stores the referrer for a page in Bookmarks or History, maybe this action should be available there too and the terminology unified. If you return to a page or download in Places later and it's become a 404, you want to go to the page from where you clicked on it to see if it's been replaced. When you File > Save a web page it triggers a download. Should anything happen if the page you save is one that you've bookmarked or is otherwise in your history? E.g. it might be nice if History and Bookmarks in Places knew that you've saved/downloaded the page (possibly more than once); or e.g. prefill the tags for the download. If you have both downloaded and bookmarked a URL it would be nice if it showed up twice in Library search, but in current Nightly the download appears in History search, and you can't search History and Bookmarks at the same time. https://wiki.mozilla.org/User:P.A./Panel-based_Download_Manager#Library_window touches on these issues but perhaps not in sufficient detail.

Are you going make current interaction in the download windows available in the download in the library?

Hey Felipe, if you need to do refactorings in the Library code, feel free to, just split things in parts and I'll gladly review them, since that code is pretty old and may be confusing.

This could be an interesting way to handle the files in the library : https://lh4.googleusercontent.com/-tr5kG5nzwfM/Tld5BVtvVpI/AAAAAAAACBY/dRlu9fr_yP8/s800/TemporaryStorage5_0001_FileBrowser_ManyDownloads_ShowAll.png

As the Library->Downloads table is shown in Nightly atm (2012-02-22), it is not the most useful or informative. I miss some fields (Download-Start, -End, DL-Target, DL-startet-from-URL, Speed) just to name a few. IMO our own KaiRo has it done right in his Add-On: "Jökulsárlón Download Manager", see https://addons.mozilla.org/en-US/firefox/addon/download-manager/ - clear table, maxima information, maxima usability. From me a big thanks to KaiRo.

This bug is much more important now as the ability to use "Go to Download Page" has been removed from the new downloads panel.

The broken state of the download library window made me switch back to Aurora. Nightly is unusable until this is fixed.

Note that this UI should probably not list the downloads initiated in private browsing mode (see bug 722859).

This should land ahead of everything else. It allows setting customized views for particular places uris.

@Mano: I'm no guru at all, so please take my comments with a grain of salt. in Attachment #640085 [details] [diff] I've detected at least one error. @@ -130,19 +131,19 @@ this._forwardHistory.splice(0, this._forwardHistory.length); } this._location = aLocation; this._places.selectPlaceURI(aLocation); if (!this._places.hasSelection) { // If no node was found for the given place: uri, just load it directly - this._content.place = aLocation; + ContentArea.currentPlace = aLocation; } - this.onContentTreeSelect(); + this.updateDetialsPane(); ==================^^^^^^ here would "this.updateDetailsPane();" make more sense, not "ia", but "ai" Otherwise a question: does this enable the possibility for separate view definition not only for "Downloads" but also different views for "History" and "Bookmarks"? Some fields I'd like to see in Bookmarks, make no sense in History and also the other way round. So, fully separate view definitions for all parts of the Library would be the "full price" goal IMHO.

(In reply to Michael Foerster from comment #17) > @Mano: > I'm no guru at all, so please take my comments with a grain of salt. > in Attachment #640085 [details] [diff] [diff] I've detected at least one error. > > @@ -130,19 +131,19 @@ > this._forwardHistory.splice(0, this._forwardHistory.length); > } > > this._location = aLocation; > this._places.selectPlaceURI(aLocation); > > if (!this._places.hasSelection) { > // If no node was found for the given place: uri, just load it > directly > - this._content.place = aLocation; > + ContentArea.currentPlace = aLocation; > } > - this.onContentTreeSelect(); > + this.updateDetialsPane(); > ==================^^^^^^ > here would "this.updateDetailsPane();" make more sense, not "ia", but "ai" > Thanks for the alert :) Fixed! > Otherwise a question: does this enable the possibility for separate view > definition > not only for "Downloads" but also different views for "History" and > "Bookmarks"? > Some fields I'd like to see in Bookmarks, make no sense in History and also > the other way round. It should be easy to do next, yes, do you know of a bug filed about this issue?

(In reply to Mano from comment #19) > > Otherwise a question: does this enable the possibility for separate > > view definition not only for "Downloads" but also different views for > > "History" and "Bookmarks"? > > Some fields I'd like to see in Bookmarks, make no sense in History and > > also the other way round. > > It should be easy to do next, yes, do you know of a bug filed about this > issue? Hmm, sorry not to my knowledge. To bring this up here and now was just the most logical thing to do. My train of thought was just: OK this is nice, but with just a little more forethought it could be much more useful. I'm a big fan of thinking ahead and doing it right the first time, and your work looked just so nice .... OK, back to topic: Is there a overview of fields specific to downloads? I'd like to rise the issue of: Download-Start-Time, -End-time(stored in last visited AFAIK), DL-Target(local-location), DL-startet-from-URL, and DL-Size. Will the places-db get a new table (most simple, more or less a integration of downloads.sqlite) or will there be more 'ANNO' abuse? As it is most of the DL -specific data just does not fit into the history (moz_places) table, and bookmarks already have their on table (moz_bookmarks). A table moz_downloads would be logical, but this is a bigwig political decision.

Comment on attachment 640132 [details] [diff] [review] Part 1 - Allow multiple views in the library Review of attachment 640132 [details] [diff] [review]: ----------------------------------------------------------------- so, I like it, though it's hard to evaluate it for a complete review without seeing the new view and how it works with at least 2 views. Though it's clearly positive. ::: browser/components/places/content/browserPlacesViews.js @@ +18,5 @@ > PlacesViewBase.prototype = { > // The xul element that holds the entire view. > _viewElt: null, > + > + get associatedElement() this._viewElt, I wonder if wouldn't be better to keep the old viewElt name, just for brief add-on compatibility reasons.. no strong feelings though. ::: browser/components/places/content/places.js @@ +273,5 @@ > * special action, since they're related to selection. > * @param aEvent > * The mouse event. > */ > + onPlacesListClick: function PO_onTreeClick(aEvent) { didn't change the function name @@ +1370,5 @@ > +let ContentArea = { > + init: function CA_init() { > + this._deck = document.getElementById("placesViewsDeck"); > + this._specialViews = new Map(); > + ContentTree.init(); I suppose other special views may have an init, so maybe there should be a more generic registrazione that invokes the init() method, and here we should just use that to register the default view (may with a special query string like "default"? @@ +1378,5 @@ > + function CA_getContentViewForQueryString(aQueryString) { > + if (this._specialViews.has(aQueryString)) > + return this._specialViews.get(aQueryString); > + return ContentTree.view; > + }, please add some documentation on what constitutes a view and how to handle/define the query string. IS the query string just the place uri or an additional query string on the place uri? since I don't know can't tell if this is properly named :/ ::: browser/components/places/content/tree.xml @@ +57,5 @@ > </property> > > + <property name="associatedElement" > + readonly="true" > + onget="return true;"/> return true?

s/registrazione/registration/, sorry, I was able to put an italian word there without noticing it :)

Separate out the controller code in order to allow multiple download views. I also cleaned up the code a little bit. I think it's somewhat easier to follow now. I will add back some comments later on.

Including the Services.jsm change.

Comment on attachment 651222 [details] [diff] [review] Downloads Commands Controller While I like the idea of reusing the controller code, what I don't understand here is the new structure of the DownloadsCommand object, that replaces DownloadsViewItemController in a way that is far more tightly coupled with the view itself. For example, DownloadsViewItemController was designed to be able to work with multi-selection lists, while the current code cannot. This seems strange to me as a preliminary step for sharing code with the Library, because the Library will need a multi-selection list to manage more downloads at once, like we do with the existing Places views. By the way, the Downmloads panel used to be multi-selection in the beginning, I can point to the relevant patches if needed.

(In reply to Paolo Amadini [:paolo] from comment #25) > Comment on attachment 651222 [details] [diff] [review] > Downloads Commands Controller > > While I like the idea of reusing the controller code, what I don't understand > here is the new structure of the DownloadsCommand object, that replaces > DownloadsViewItemController in a way that is far more tightly coupled with > the > view itself. For example, DownloadsViewItemController was designed to be able > to work with multi-selection lists, while the current code cannot. > Each command takes the view, not the data item, so that's easy fixable... However, I guess I should change selectedDownloadDataItem to selectedDownloadDataItem/s/. Makes sense?

(In reply to Mano from comment #26) > Each command takes the view, not the data item, so that's easy fixable... > However, I guess I should change selectedDownloadDataItem to > selectedDownloadDataItem/s/. Makes sense? The old code used to enumerate the items outside of the item's controller, so that each command didn't have to contain the enumeration logic itself. Whether a command could be enabled for a group of items was also computed outside of the item. But, of course fuctionality is the same even if each command accesses a list called aView.selectedDownloadDataItems.

That's (enumerating within the command) in parallel to how places commands work (and how commands work in general, actually), so I think I'll go with this option.

Comment on attachment 651222 [details] [diff] [review] Downloads Commands Controller Please get preliminary positive feedback from paolo on this part, I see there is already some feedback to be fixed, I'm setting the flag but is intended to be set on the next version of the patch.

so, do you still need my review on that patch or should I wait the newer version?

No-no. I'll ask Paolo for feedbacks and reviews on the next iteration of patches. You'll do the final review, thanks :)

There are some unfortunate focus bugs in the downloads panel which break some parts of this code, but I know about them and will get to them before this lands.... asking for feedback.

Comment on attachment 659786 [details] [diff] [review] Downloads Commands Controller Review of attachment 659786 [details] [diff] [review]: ----------------------------------------------------------------- This is interesting but I still think we have some redundancy we could avoid, see the last part of the detailed review below. I would also like to see how this fits into the overall library window patch. I think a preliminary, testable WIP of the Downloads view in the library would help in getting more context for this patch. ::: browser/components/downloads/content/downloads.js @@ +414,5 @@ > */ > const DownloadsView = { > + initialize: function DV_initialize() { > + this._controller = new DownloadsViewController(DownloadsView); > + this.richListBox.controllers.appendController(this._controller); The controller used to be registered on the entire window, with special code to limit its scope. This might be something I just overlooked, a workaround for making things work, or a leftover from when the space bar was handled as a global window key definition. Does the panel still work in the same way with the controller registered on the listbox? (It should be easy to test, even without this patch applied) @@ +616,5 @@ > return this._invisibleViewItem; > }, > > + ownerWindow: window, > + get selectedDownloadDataItems() { nit: can be just selectedDataItems (we're already in a DownloadsView object). @@ +622,5 @@ > + if (element) { > + let downloadId = element.getAttribute("downloadId"); > + return [DownloadsCommon.data.dataItems[downloadId]]; > + } > + return null; Return an empty array when there are no elements. @@ +709,5 @@ > return; > } > > if (aEvent.charCode == " ".charCodeAt(0)) { > + let dataItem = this.selectedDownloadDataItems[0]; This can fail if there is no selected item. ::: browser/components/downloads/content/downloadsOverlay.xul @@ +19,5 @@ > oncommand="goDoCommand('downloadsCmd_doDefault')"/> > + <command id="downloadsCmd_pause" > + oncommand="goDoCommand('downloadsCmd_pause')"/> > + <command id="downloadsCmd_resume" > + oncommand="goDoCommand('downloadsCmd_resume')"/> Thanks for separating these commands, it's clearer now. ::: browser/components/downloads/src/DownloadsViewController.jsm @@ +22,5 @@ > + cmd_delete: { > + isCommandEnabled: function(aView) aView.selectedDownloadDataItems.length > 0, > + doCommand: function DCM_delete_doCommand(aView) { > + DownloadCommands.cancel.doCommand(aView); > + for (let dataItem of aView.selectedDownloadDataItems) { After seeing this code, I still think we're being redundant in all commands with the aView argument, item iteration, and the check that determines if there is at least one item selected. Technically, also, a controller should not call into the view object, but only into the model object (but it's not the main reason for suggesting to change this, we're not applying the MVC model strictly). I appreciate having the check for whether the command is enabled near the command implementation, but we could do this in another way to avoid the redundancy. For example, a DownloadsDataItemController constructed with the data item. To differentiate commands, you could use objects that have the same base prototype (something like objects derived from DownloadsDataItemCommand), or just call into the isCommandEnabled, doCommand functions with "this" pointing to the DownloadsDataItemController. @@ +78,5 @@ > + } > + > + if (showAlert) { > + const strings = DownloadsCommon.strings; > + let name = dataItem.target; dataItem seems undefined here. @@ +295,5 @@ > + onEvent: function() { }, > + > + updateCommands: function() { > + Object.keys(DownloadCommands).forEach(function(aCommand) { > + aView.ownerWindow.goUpdateCommand(aCommand); I see this patch still has no comments as it's a preliminary feedback request, but when we get to the comments phase, we should remember that this controller object requires the ownerWindow to have loaded the global utilities (where goUpdateCommand is defined).

any ETA for a first testable patch?

Unfortunately all I can say is "this week".

Update: The good news is that, as for functionality, everything is now implemented except: 1. Undo/Redo - minor issue, imo. 2. Search of downloads which are not saved in history. This means that if one cleaned the history, or is using private browsing, searching the downloads view will results in 0 results. This is going to be quite confusing. It would help if the new downloads implementation supported some kind of search, but for now there's not much I can do about this. The not-so-good-not-so-bad news is that I've a ton of code cleanup and testing to do now. Targeting early Wednesday for a patch, hopefully that's the very last delay. I'm sorry this is taking so long. Meanwhile, I'll detail the important implementation details and alchemy techniques used to achieve the "unified" downloads view, and the shortcomings of this approach: 1. There's no visual distinction between past downloads and session downloads. However, session-downloads are always shown above past downloads. 2. The custom downloads view has a uri->"download element shells" map. A "download element shell" is simply a js object that manages one richlistitem for a downloaded uri. You can set either a places-node on it, or a download-data-item, or both. Now: 2.1. If only one of them is set - either because the download was completed in another session, meaning it's no a "session download", or if there's no history entry for some session download - we just show what we have. For past downloads, it means that we have to look at some annotations. For session downloads, we use the data set on the DownloadDataItem object. 2.2. If both are set, we take the download-data-item data. 2.3. Every time the places node or the downloaddataitem is reset/removed for a uri, we re-evaluate what we want to show. When both are removed (i.e. there's no history entry *and* there's no DownloaDdataItem), the richlistitem (along with its "shell") are removed entirely. 3. If there's a places-node for a given download-uri, and we're notified about a new download data item, we reuse the shell for the places node. 4. However, if *another* download data item for the same uri comes in, another richlistitem is created (along with another shell). Both shells will point to the same places node. 5. To be clear, another places node *cannot* came in, because the downloads places query is set to RESULT_AS_URI. As for commands, controllers: 1. There's a "proxy" controller that delegates each command do/update call to either the places controller or the downloads controller (It creates both). 2. The same context menu is used either way. 3. The places controller now implements some downloads-commands, but the regular places context menu doesn't list them, so it shouldn't be an issue. 4. Undo/Redo is a no-go at this point. About the download state for history downloads: 1. Because there cannot be more than one entry for an history download, new downloads of the same uri override old downloads. This means that if a file was downloaded successfully once, and, later, the same file was downloaded with a failure, the data about the download that made it would be lost. 2. If the same file is download few times simultaneously, the behavior is going to be quite embracing for us. However, that's an edge case. Multiple selection issues: 1. It's enabled. The proxy-controller will pick the right controller for each richlistitem. 2. The situation with undo-redo makes this somewhat buggy. One could (later) undo the commands performed on places items, but not the commands performed on downloads. Other than that, you'll notice there's quite a lot of code duplication now. While tempting, it turned to be very hard to get the code shared in the initial landing. I'll get back to this once the new download system isn't a moving target.

(In reply to Mano from comment #38) > Update: That's very good news overall! :-)

(In reply to Mano from comment #38) > 1. Undo/Redo - minor issue, imo. I agree, really minor (I'm not even sure we want that support). > 2. Search of downloads which are not saved in history. Follow-up bug, it may be a problem especially in pb mode. Though, since I don't expect people to download tens of files per day the search, and we explicitly never wanted the DM to be good for mass downloading, search has a minor role. So not blocking. > The not-so-good-not-so-bad news is that I've a ton of code cleanup and > testing to do now. Targeting early Wednesday for a patch, hopefully that's > the very last delay. I'm sorry this is taking so long. It's fine. Though considered the size of the change, I think we should also evaluate if shipping the feature on top of the old DM window (and take the Library view at the next release) is a plausible choice. Clearly this also depends on ETA of the other design blockers we still have. > 1. Because there cannot be more than one entry for an history download, new > downloads of the same uri override old downloads. This means that if a file > was downloaded successfully once, and, later, the same file was downloaded > with a failure, the data about the download that made it would be lost. This may be annoying, though it's again a follow-up thing. The most common case is that the last download wins and that works. > 2. If the same file is download few times simultaneously, the behavior is > going to be quite embracing for us. However, that's an edge case. likely-do-not-care > Multiple selection issues: > 2. The situation with undo-redo makes this somewhat buggy. One could (later) > undo the commands performed on places items, but not the commands performed > on downloads. Didn't you say we do not support undo-redo here? by "later" you mean "once we implement it"? Btw, good news indeed.

Any updates on this? A patch we could look at?

Still Missing: * Winstripe/Gnomestripe support (easy) * copy-referrer for past downloads * most-important: "retry" for past-downloads * Date-time for downloads (easy) * Comments, a lot * Testing, a ton.

Could you please clarify the status of previous patches attached here, are they obsoleted by the latest one?

Yes, they are. The patch for "special-views" in the Library is included in this patch, and the rewritten downloads-controller will have to happen in a follow up, if at all.

OK, things I've noticed don't work (some of these were already reported by you) - Organize / Select All, Copy, ... - Context menu / Go to Download Page, Copy Download Link - The details pane (we may even just collapse it) - Views Menu entries are pointless (Show columns, Sort) - on Mac I don't see the downloads icons for some reason, all items have no icons, works fine on Win, regression? - there is a border around the view (also in pinstripe), while other views don't have a border, it should be removed - we may want to introduce some separator between items, like the old DM window - should change Show All Downloads and Downloads to open the Library (if useToolkitUI is false)

Comment on attachment 680751 [details] [diff] [review] checkpoint for feedback Review of attachment 680751 [details] [diff] [review]: ----------------------------------------------------------------- well the approach looks ok, there's lot of code but not too much. The only design problem we have here, that is basically valid for all of the views, is that this is totally synchronous in regards to annotations and file operations... If these accesses would be fake-async, in future would be easier to swap them, though it's something we may look at later, doing this change now may delay this by quite a while. ::: browser/components/downloads/src/DownloadsCommon.jsm @@ +130,5 @@ > /** > + * Returns the status-text and status-tip to be displayed for the given > + * data item. > + */ > + getStatusTextAndTipForDataItem: function(aDataItem) { I suppose this may be modified to be usable in _updateStatusLine http://mxr.mozilla.org/mozilla-central/source/browser/components/downloads/content/downloads.js#908 Though the needs may be different (for example in the manager we want to show the ratio speed regardless), so we could do this later if complicated @@ +255,5 @@ > * at present. > */ > + get indicatorData() DownloadsIndicatorData, > + > + openDownloadedFile: function(aFile, aWindow) { Again, I suppose your scope is to replace the various commands here http://mxr.mozilla.org/mozilla-central/source/browser/components/downloads/content/downloads.js#1184 but this patch doesn't yet do that ::: browser/components/places/content/downloadsView.js @@ +18,5 @@ > + .getService(Components.interfaces.nsIFileProtocolHandler); > + return fileProtocolHandler.getFileFromURLSpec(aFileURI); > +} > + > +function DownloadElementShell(aRichListBox, aDataItem, aPlacesNode) { remember to add documentation to the various parts @@ +100,5 @@ > + get _targetFileURI() { > + if (this._dataItem) > + return this._dataItem.file; > + > + return this._getAnnotation(DESTINATION_FILE_URI_ANNO, ""); May we cache this? it's unlikely to change. @@ +113,5 @@ > + } > + catch(ex) { } > + > + // Fallback to the places title, or, at last, to the download uri. > + return this._placesNode.title || this.downloadURI; May we cache this? it's unlikely to change. ::: browser/components/places/content/places.css @@ +22,5 @@ > + > +.download-state:-moz-any( [state="6"], /* Blocked (parental) */ > + [state="8"], /* Blocked (dirty) */ > + [state="9"]) /* Blocked (policy) */ > + .downloadTypeIcon:not(.blockedIcon), IS this the same content that is in the newly added browser/components/places/content/download.css? Can't we re-use that one? ::: browser/components/places/content/places.js @@ +7,5 @@ > > +const DOWNLOADS_QUERY = "place:transition=" + > + Components.interfaces.nsINavHistoryService.TRANSITION_DOWNLOAD + > + "&sort=" + > + Components.interfaces.nsINavHistoryQueryOptions.SORT_BY_DATE_DESCENDING; iirc you can use Ci in the library window @@ +281,5 @@ > + updateDetailsPane: function PO_updateDetailsPane() { > + let view = PlacesUIUtils.getViewForNode(document.activeElement); > + if (view) { > + let selectedNodes = view.selectedNode ? > + [view.selectedNode] : view.selectedNodes; the second option could be an explicit null @@ +661,5 @@ > + if (ContentArea.currentView.result) { > + let rootNode = ContentArea.currentView.result.root; > + if (rootNode.containerOpen) > + itemsCount = rootNode.childCount; > + } Maybe could add an helper to currentView to return the childCount? I can see other situations where the same need may rise. Btw, let's try to avoid "items" due to the common miscomprehension with bookmarks, let's use entriesCount? childCount? @@ +769,5 @@ > } > > + let currentView = ContentArea.currentView; > + let currentOptions = PlacesUtils.asQuery(currentView.result.root) > + .queryOptions; why not keep using getCurrentOptions, it seems to do the same ::: browser/components/places/content/tree.xml @@ +57,5 @@ > </property> > > + <property name="associatedElement" > + readonly="true" > + onget="return true"/> return true? ::: browser/components/places/jar.mn @@ +9,5 @@ > * content/browser/places/places.xul (content/places.xul) > * content/browser/places/places.js (content/places.js) > +* content/browser/places/downloadsView.js (content/downloadsView.js) > +* content/browser/places/download.xml (content/download.xml) > +* content/browser/places/download.css (content/download.css) doesn't look like all of these need preprocessing

you should also reimplement bug 712421 in this view

Does anyone have any estimates on how close this is to landing? Thanks!

I would say about 8 days, including reviews. Revised patch will be posted on Wednesday, with no drastic changes from the latest revision, which Marco knows pretty well by now. The only thing I still have not implemented at all is theme changes for winstripe and gnomestripe.

That's great, thanks a lot, Mano!

Lots of little bug fixes, quite a bit of missing functionality is now implemented. I've not included the "Clear Downloads" feature here yet (will do in the very next iteration (the backend is ready). I've not included theme changes for windows. "Open Referrer" for past downloads isn't implemented either. Not yet asking for code review, but functionality review (on mac...) would help a lot.

(In reply to Mano from comment #52) > Created attachment 686667 [details] [diff] [review] > another checkpoint > > Lots of little bug fixes, quite a bit of missing functionality is now > implemented. I've not included the "Clear Downloads" feature here yet (will > do in the very next iteration (the backend is ready). I've not included > theme changes for windows. "Open Referrer" for past downloads isn't > implemented either. > > Not yet asking for code review, but functionality review (on mac...) would > help a lot. So a few things to note on OSX: 1) The download buttons (cancel, retry, open containing folder), are barely visible for the selected item(s): http://i.imgur.com/hRfse.png 2) For in-progress downloads, the time remaining / rate in a download is repeated twice below the progress bar. (visible in the last screenshot)

Also, I notice that if I remove an item from the list of downloads via the Downloads Panel, the item remains in the Places view. (It's removed from the old toolkit downloads window though).

Mano: Is there anything else besides testing that I can do to help push this along? -Mike

I've taken the liberty of spinning up a try-build for Mano's latest patch - it'll come in here: https://tbpl.mozilla.org/?tree=Try&rev=f4549021ebc7

Has someone a screenshot of what it currently looks ? Will this need visual polish later ? I was also asking myself about the future lack of consistency between this and the rest of the library. Won't this be a problem ?

(In reply to Mike Conley (:mconley) from comment #54) > Also, I notice that if I remove an item from the list of downloads via the > Downloads Panel, the item remains in the Places view. (It's removed from the > old toolkit downloads window though). That's expected behavior, removing a download from a current notification does not also remove it from history. (The old Downloads window will not be visible.)

(In reply to Guillaume C. [:ge3k0s] from comment #57) > Will this need visual polish later ? yes, definitely, as any new feature going into the product it will need polish fixes. > I was also asking myself about the future lack of consistency between this > and the rest of the library. Won't this be a problem ? There's no lack of consistency, it's just the Library right pane become pluggable, so depending on the kind of data it may get different view. If you think to Windows explorer for example, you get a different view of image folders compared to common file folders.

Mano, supposing this "somehow works", I think may even be fine to land it and work on it all-hands to clean it up. If the architecture is not undergoing a major revamp, it's just matter of plugging fixes here and there. We could parallelize work on features and themes that way.

Comment on attachment 686667 [details] [diff] [review] another checkpoint Review of attachment 686667 [details] [diff] [review]: ----------------------------------------------------------------- Some drive-by comments, was mostly checking if there was some blocking architectural issue. Something we may need is to be able to load this view into a tab with only session downloads (no history), that will be the initial behavior in private browsing mode. Do you think would be hard to have that out of this? ::: browser/components/downloads/src/DownloadsCommon.jsm @@ +194,5 @@ > + status = s.statusSeparator(firstPart, displayDate); > + statusTip = s.statusSeparator(fullHost, fullDate); > + } > + > + return [status, statusTip || status]; Some trailing spaces @@ +296,5 @@ > + // Actually open the file. > + try { > + let launched = false; > + try { > + let mimeInfo = this.dataItem.download.MIMEInfo; the .download property is going to change to a .getDownload(callback), in bug 801232, since we can't rely anymore on the download id. Should be trivial to fix this callpoint though. @@ +339,5 @@ > + }, > + > + cancelDownload: function(aDataItem) { > + if (aDataItem.inProgress) { > + Services.downloads.cancelDownload(aDataItem.downloadId); We can't rely anymore on .downloadId in per-window-pb mode, this will have to get the download and call .cancel() on it... It will be async. @@ +354,5 @@ > + }, > + > + removeDownload: function(aDataItem) { > + this.cancelDownload(aDataItem); > + Services.downloads.removeDownload(aDataItem.downloadId); as well as here, must become async ::: browser/components/places/content/download.css @@ +33,5 @@ > + .downloadCancel, > + > +.download-state:not(:-moz-any([state="2"], /* Failed */ > + [state="3"]) /* Canceled */) > + .downloadRetry, We slightly changed the style recently in the downloads component, we had to put button into a stack and they now use visibility: hidden, since otherwise during antirivus scans no button was shown and the layout was flickering briefly. If we really need to copy all of this would be nice to keep them in sync. ::: browser/components/places/content/download.xml @@ +45,5 @@ > + command="downloadsCmd_retry" > + tooltiptext="&cmd.retry.label;"/> > + <xul:button class="downloadButton downloadShow" > + command="downloadsCmd_show" > + tooltiptext="&cmd.show.label;"/> ditto on the <stack>, please check the new binding ::: browser/components/places/content/downloadsView.js @@ +5,5 @@ > +/** > + * THE PLACES VIEW IMPLMENTED IN THIS FILE HAS A VERY PARTICULAR USE CASE. IT'S > + * HIGHLY RECOMMENDED NOT TO EXTEND IT FOR ANY OTHER USE CASES OR RELY ON IT AS > + * AN API. > + */ typo: implmented a trailing space on first line @@ +31,5 @@ > + > +function GetFileForFileURI(aFileURI) > + Cc["@mozilla.org/network/protocol;1?name=file"]. > + getService(Ci.nsIFileProtocolHandler). > + getFileFromURLSpec(aFileURI); This has a single use... not sure it's worth its own helper. @@ +81,5 @@ > + // The data item for the download > + get dataItem() this._dataItem, > + > + set dataItem(aValue) { > + if (this._dataItem = aValue) { this assignment in the condition is scary,scary,scary @@ +85,5 @@ > + if (this._dataItem = aValue) { > + this._wasDone = this._dataItem.done; > + this._wasInProgress = this._dataItem.inProgress; > + } > + else if (this.elementNode) { this looks wrong, should likely be .element @@ +90,5 @@ > + this._wasInProgress = false; > + this._wasDone = this._state == nsIDM.DOWNLOAD_FINISHED; > + } > + > + this._initUI(); more than a initUI this looks like a refreshUI (better naming please, UI is too much generic)... it's invoked like that as far as I can tell @@ +138,5 @@ > + catch(ex) { > + if (!aDefaultValue) > + throw ex; > + } > + this._annotations.set(aAnnotation, aDefaultValue); I doubt this works since we are early returning PlacesUtils.annotations.getPageAnnotation @@ +210,5 @@ > + else { > + if (this._file.exists()) { > + state = this._fileSize > 0 ? nsIDM.DOWNLOAD_FINISHED : nsIDM.DOWNLOAD_FAILED; > + } > + else { wouldn't be the same to else if/else? @@ +212,5 @@ > + state = this._fileSize > 0 ? nsIDM.DOWNLOAD_FINISHED : nsIDM.DOWNLOAD_FAILED; > + } > + else { > + // xxx > + state = nsIDM.DOWNLOAD_CANCELED; xxx comment @@ +229,5 @@ > + let s = DownloadsCommon.strings; > + switch (this._state) { > + case nsIDM.DOWNLOAD_FAILED: > + return s.stateFailed; > + break; if we early return we may avoid the breaks @@ +247,5 @@ > + // For completed downloads, show the file size (e.g. "1.5 MB") > + if (this._fileSize > 0) { > + let [size, unit] = DownloadUtils.convertByteUnits(this._fileSize); > + return s.sizeWithUnits(size, unit); > + } though here since the return is confitional a break would be welcome some trailing spaces here around @@ +277,5 @@ > + // to update the progress-meter. > + if (!this._dataItem) > + return; > + > + // Copied from updateProgress in downloads.js. trailing spaces above @@ +293,5 @@ > + else { > + // This is a running download of which we know the progress. > + this._element.setAttribute("progressmode", "normal"); > + this._element.setAttribute("progress", this._dataItem.percentComplete); > + } trailing spaces @@ +390,5 @@ > + return this._dataItem != null; > + } > + }, > + > + _getTargetFileOrPartFileIfExists: function DES__getTargetFileOrPartFileIfExists() { trailing spaces above @@ +399,5 @@ > + return this._dataItem.partFile; > + return null; > + }, > + > + _retryAsHistroyDownload: function DES__retryAsHistroyDownload() { typo: Histroy (repeated after) @@ +400,5 @@ > + return null; > + }, > + > + _retryAsHistroyDownload: function DES__retryAsHistroyDownload() { > + // TODO: save in the right location (the current saveURL api deos not allow this) typo: deos @@ +401,5 @@ > + }, > + > + _retryAsHistroyDownload: function DES__retryAsHistroyDownload() { > + // TODO: save in the right location (the current saveURL api deos not allow this) > + saveURL(this.downloadURI, this._displayName, null, true, true, undefined, document); the document here is the Library document, this is bad cause I'm not sure if the Library inherits private browsing status from the browser window that opened it... I'm not sure how we are going to handle this, what we'd want is the library in a tab, so it's document is the browser window, but that's a bit longer term... this may actually be a privacy hit, not sure which document we may decide to use, we should probably first figure the window this is going to open from. @@ +429,5 @@ > + case "downloadsCmd_retry": > + if (this._dataItem) > + Services.downloads.retryDownload(this._dataItem.downloadId); > + else > + this._retryAsHistroyDownload(); same typo as above: Histroy @@ +435,5 @@ > + case "downloadsCmd_pauseResume": { > + if (this._dataItem.paused) > + Services.downloads.resumeDownload(this._dataItem.downloadId); > + else > + Services.downloads.pauseDownload(this._dataItem.downloadId); ditto in the fact we can't rely on downloadId, this will again have to getDownload(function (aDownload) aDownload.resume()), or pause() That will require bug 815941. @@ +482,5 @@ > + } > +}; > + > +/** > + * A Downloads Places View is aplaces view designed to show a places query typo: aplaces @@ +523,5 @@ > + if (this._downloadElementsShellsForURI.has(aURI)) { > + let downloadElementShells = this._downloadElementsShellsForURI.get(aURI); > + for (let des of downloadElementShells) { > + aCallback(des); > + } trailing spaces @@ +801,5 @@ > + applyFilter: function() { > + throw new Error("applyFilter is not implemented by the DownloadsView") > + }, > + > + load: function(aQueries, aOptions) { trailing spaces @@ +861,5 @@ > + let data = {}; > + trans.getAnyTransferData({}, data, {}); > + let [url, name] = data.value.QueryInterface(Ci.nsISupportsString).data.split("\n"); > + if (url) > + return [Services.io.newURI(url, null, null).spec, name]; NetUtil.newURI @@ +927,5 @@ > + element._shell.doDefaultCommand(); > + } > + } > + else if (aEvent.charCode == " ".charCodeAt(0)) { > + // Pasue/Resume every selected download typo: pasue @@ +938,5 @@ > +}; > + > +function goUpdateDownloadCommands() { > + for (let command of DOWNLOAD_VIEW_SUPPORTED_COMMANDS) > + goUpdateCommand(command); please brace loops ::: browser/components/places/content/places.css @@ +77,5 @@ > +.download-state[state="7"] .downloadCommandsSeparator > + > +{ > + display: none; > +} same as browser/components/places/content/download.css ? maybe include? is this repetition really needed? ::: browser/components/places/content/places.js @@ +769,5 @@ > } > > + let currentView = ContentArea.currentView; > + let currentOptions = PlacesUtils.asQuery(currentView.result.root) > + .queryOptions; getCurrentOptions? ::: browser/components/places/content/tree.xml @@ +57,5 @@ > </property> > > + <property name="associatedElement" > + readonly="true" > + onget="return true"/> still wrong return value ::: browser/components/places/jar.mn @@ +9,5 @@ > * content/browser/places/places.xul (content/places.xul) > * content/browser/places/places.js (content/places.js) > +* content/browser/places/downloadsView.js (content/downloadsView.js) > +* content/browser/places/download.xml (content/download.xml) > +* content/browser/places/download.css (content/download.css) still too much preprocessing

(In reply to Mike Conley (:mconley) from comment #54) > Also, I notice that if I remove an item from the list of downloads via the > Downloads Panel, the item remains in the Places view. (It's removed from the > old toolkit downloads window though). (In reply to Paolo Amadini [:paolo] from comment #58) > That's expected behavior, removing a download from a current notification > does > not also remove it from history. (The old Downloads window will not be > visible.) So, discussed a bit with Mano, and you both are right. This IS the expected behavior, but the command in the panel is extremely confusing for the user. So I filed bug 817006 to handle this problem.

Mano: Any update on this? I thought this was supposed to land last Friday... -Mike

Yes. Final patch coming in the next few hours.

Not asking for review yet. I need to discuss some bugs with Marco.

Comment on attachment 690350 [details] [diff] [review] use the new async api, addree (most) comments, finalize structure Mike, could you look at the changes to downloads.js and DownloadsCommon.jsm and see if they make sense to you? The comments deserve some love for sure, and I should also (later) rename DownloadsDataItem to reflect the fact that it's no long just a "data" item, but the notion in general should be clear.

Comment on attachment 690350 [details] [diff] [review] use the new async api, addree (most) comments, finalize structure Review of attachment 690350 [details] [diff] [review]: ----------------------------------------------------------------- Moving the dataItem manipulation functions from downloads.js into DownloadsCommon.jsm where the Places view can make use of them makes sense to me. ::: browser/components/downloads/src/DownloadsCommon.jsm @@ +64,5 @@ > const nsIDM = Ci.nsIDownloadManager; > > const kDownloadsStringBundleUrl = > "chrome://browser/locale/downloads/downloads.properties"; > + nit: trailing whitespace @@ +405,5 @@ > + }, > + > + /** > + * Opens a downloaded file. > + * If you've a data-item, you should call dateItem.openLocalFile. Nit - should be "If you have a dataItem, you should call dataItem.openLocalFile". @@ +460,5 @@ > + return; > + } > + } > + catch(ex) { } > + Nit: whitespace @@ +471,5 @@ > + // If launch fails, try sending it through the system's external "file:" > + // URL handler. > + Cc["@mozilla.org/uriloader/external-protocol-service;1"]. > + getService(Ci.nsIExternalProtocolService). > + loadUrl(NetUtil.newURI(aFile)); Nit: whitespace @@ +477,5 @@ > + }, > + > + /** > + * Show a donwloaded file in the system file manager. > + * If you have a data-item, use dataItem.showLocalFile. data-item -> dataItem @@ +499,5 @@ > + // If launch also fails (probably because it's not implemented), let > + // the OS handler try to open the parent. > + Cc["@mozilla.org/uriloader/external-protocol-service;1"]. > + getService(Ci.nsIExternalProtocolService). > + loadUrl(NetUtil.newURI(parent)); Nit: whitespace @@ +1342,5 @@ > + */ > + showLocalFile: function DDI_showLocalFile() { > + DownloadsCommon.showDownloadedFile(this.localFile); > + }, > + Nit: whitespace @@ +1350,5 @@ > + */ > + togglePauseResume: function DDI_togglePauseResume() { > + if (!this.inProgress || !this.resumable) > + throw new Error("The given download cannot be paused or resumed"); > + Nit: whitespace @@ +1396,5 @@ > + */ > + cancel: function() { > + if (!this.inProgress) > + throw new Error("Cannot cancel this download"); > + Nit: whitespace

We want to start fixing things incrementally, so we will land this preffef-off and enable it sometime in the next few days.

Comment on attachment 691398 [details] [diff] [review] landable preffed-off patch Review of attachment 691398 [details] [diff] [review]: ----------------------------------------------------------------- There is clearly polish to do, and theming is incomplete, but globally the approach is fine and we need to start working all-hands on this, so I'd say to land it. It's preffed off, so we can evaluate major issues before enabling it for everyone. ::: browser/components/downloads/src/DownloadsCommon.jsm @@ +402,5 @@ > + }, > + > + /** > + * Opens a downloaded file. > + * If you've a dataItem, you should call dateItem.openLocalFile. typo: dateItem @@ +408,5 @@ > + * the downloaded file to be opened. > + * @param [optional] aMimeInfo > + * the mime type info object. > + * @param aOwnerWindow > + * the window with which this action is associated. wouldn't be better to move the optional argument as last param? @@ +412,5 @@ > + * the window with which this action is associated. > + */ > + openDownloadedFile: function DC_openDownloadedFile(aFile, aMimeInfo, aOwnerWindow) { > + // Confirm opening executable files if required. > + if (aFile.isExecutable()) { since you are now exposing this as an API in the module, I'd like some arguments check and throw: typeof aFile, aOwnerWindow... @@ +468,5 @@ > + // If launch fails, try sending it through the system's external "file:" > + // URL handler. > + Cc["@mozilla.org/uriloader/external-protocol-service;1"]. > + getService(Ci.nsIExternalProtocolService). > + loadUrl(NetUtil.newURI(aFile)); indent as Cc["@mozilla.org/uriloader/external-protocol-service;1"] .getService(Ci.nsIExternalProtocolService) .loadUrl(NetUtil.newURI(aFile)); @@ +469,5 @@ > + // URL handler. > + Cc["@mozilla.org/uriloader/external-protocol-service;1"]. > + getService(Ci.nsIExternalProtocolService). > + loadUrl(NetUtil.newURI(aFile)); > + } trailing spaces @@ +496,5 @@ > + // If launch also fails (probably because it's not implemented), let > + // the OS handler try to open the parent. > + Cc["@mozilla.org/uriloader/external-protocol-service;1"]. > + getService(Ci.nsIExternalProtocolService). > + loadUrl(NetUtil.newURI(parent)); ditto on indentation, also trailing spaces @@ +1340,5 @@ > + showLocalFile: function DDI_showLocalFile() { > + DownloadsCommon.showDownloadedFile(this.localFile); > + }, > + > + /** trailing spaces @@ +1341,5 @@ > + DownloadsCommon.showDownloadedFile(this.localFile); > + }, > + > + /** > + * Resumes if download if it's pasued, or pauses it if it's active. "Resumes the download if paused, pauses it if active." @@ +1348,5 @@ > + togglePauseResume: function DDI_togglePauseResume() { > + if (!this.inProgress || !this.resumable) > + throw new Error("The given download cannot be paused or resumed"); > + > + this.getDownload(function(aDownload) { trailing space ::: browser/components/places/content/downloadsView.js @@ +2,5 @@ > + * License, v. 2.0. If a copy of the MPL was not distributed with this file, > + * You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +/** > + * THE PLACES VIEW IMPLMENTED IN THIS FILE HAS A VERY PARTICULAR USE CASE. IT'S typo: implmented @@ +20,5 @@ > +const nsIDM = Ci.nsIDownloadManager; > + > +const DESTINATION_FILE_URI_ANNO = "downloads/destinationFileURI"; > +const DESTINATION_FILE_NAME_ANNO = "downloads/destinationFileName"; > +const DOWNLOAD_STATE_ANNO = "downloads/state_"; Why the trailing underscore? @@ +31,5 @@ > + > +function GetFileForFileURI(aFileURI) > + Cc["@mozilla.org/network/protocol;1?name=file"]. > + getService(Ci.nsIFileProtocolHandler). > + getFileFromURLSpec(aFileURI); align dots to [ @@ +178,5 @@ > + this.__file = this._dataItem.localFile; > + } > + else { > + this.__file = this._targetFileURI ? > + GetFileForFileURI(this._targetFileURI) : null; nit: indent once more @@ +432,5 @@ > + }, > + > + _retryAsHistoryDownload: function DES__retryAsHistoryDownload() { > + // TODO: save in the right location (the current saveURL api does not allow this) > + saveURL(this.downloadURI, this._displayName, null, true, true, undefined, document); We need a follow-up to properly handle this in pb mode, we can use RecentWindow.jsm and use the most recent browser window's document, since this document refers to the library and should be used only if the Library is the only open window. the followup bug number should re reported in the comment. @@ +550,5 @@ > + DownloadsCommon.getData(window.opener || window).addView(this); > + // Make sure to unregister the view if the window is closed. > + window.addEventListener("unload", function() { > + if (this._result) { > + DownloadsCommon.getData(window.opener || window).removeView(); so, first this looks bogus since in global pb mode (that we will support until per-window-pb is ready) window.opener || window status may change from pb to non pb, and then the getData calls may return different data objects at the 2 times... That means we may add view to one and try to remove it from the other... it's possible also the panel is bogus though :( Also, removeView is lacking the argument. @@ +710,5 @@ > + return; > + } > + else { > + shell.dataItem = null; > + // Move it beloe the session-download items; typo: beloe @@ +933,5 @@ > + }, > + > + _downloadURLFromClipboard: function DPV__downloadURLFromClipboard() { > + let [url, name] = this._getURLFromClipboardData(); > + saveURL(url, name || url, null, true, true, undefined, document); same here for document and pb ::: browser/components/places/content/places.css @@ +77,5 @@ > +.download-state[state="7"] .downloadCommandsSeparator > + > +{ > + display: none; > +} Some of the rules look duplicated in browser/components/places/content/download.css ::: browser/components/places/content/places.js @@ +1243,5 @@ > + return Services.prefs.getBoolPref("browser.library.useOldDownloadsView"); > + } > + catch(ex) { } > + return true; > + }, what about inverting, make the pref and the method useNewDownloadsView, default to false if doesn't exist. would make more sense below where you "&& !this._shouldUseOldDownloadsView()" @@ +1262,5 @@ > + function CA_setContentViewForQueryString(aQueryString, aView) { > + this._specialViews.set(aQueryString, aView); > + }, > + > + get currentView() PlacesUIUtils.getViewForNode(this._deck.selectedPanel), trailing spaces @@ +1312,5 @@ > + // selection is a folder or a different container type, and will > + // load its contents in tabs. > + PlacesUIUtils.openContainerNodeInTabs(node, aEvent, this.view); > + } > + } trailing spaces ::: browser/components/places/content/places.xul @@ +53,5 @@ > src="chrome://browser/content/utilityOverlay.js"/> > <script type="application/javascript" > src="chrome://browser/content/places/editBookmarkOverlay.js"/> > + <script type="application/javascript" > + src="chrome://global/content/contentAreaUtils.js"/> sigh :/ @@ +505,5 @@ > + accesskey="&cmd.goToDownloadPage.accesskey;"/> > + <menuitem command="downloadsCmd_copyLocation" > + label="&cmd.copyDownloadLink.label;" > + accesskey="&cmd.copyDownloadLink.accesskey;"/> > + </menupopup> trailing spaces ::: browser/themes/pinstripe/places/places.css @@ +203,5 @@ > opacity: 0.7; > } > + > + > +/** Downloads View **/ We need a follow-up bug to do the theming on Win and Lin. @@ +223,5 @@ > + > +#downloadsRichListBox:-moz-focusring > richlistitem.download[selected] { > + outline: 1px -moz-dialogtext dotted; > + outline-offset: -1px; > +} I think this is wrong, as well as we are evaluating it wrong on the panel... I'd say to remove it for now here. @@ +264,5 @@ > +} > + > +/*.downloadButton:-moz-focusring > .button-box { > + outline: 1px -moz-dialogtext dotted; > +}*/ this is commented out without a comment or anything explaining why

please try to land before EOW, next week we must all co-op to make this shippable.

Just a heads up - this patch is currently failing an assertion when I enable the new downloads view, and attempt to click on one of the download items. Assertion failure: mInitialized, at /media/Projects/mozilla/fx-team/widget/xpwidgets/nsTransferable.cpp:446 Backtrace: #0 0xb7731424 in __kernel_vsyscall () #1 0xb74d4c16 in nanosleep () at ../sysdeps/unix/syscall-template.S:82 #2 0xb74d4a0f in __sleep (seconds=0) at ../sysdeps/unix/sysv/linux/sleep.c:138 #3 0xb3146dac in ah_crap_handler (signum=11) at /media/Projects/mozilla/fx-team/toolkit/xre/nsSigHandlers.cpp:87 #4 0xb31500f4 in nsProfileLock::FatalSignalHandler (signo=11, info=0xbfdae8cc, context=0xbfdae94c) at /media/Projects/mozilla/objdir-fx-team-patches/toolkit/profile/nsProfileLock.cpp:190 #5 <signal handler called> #6 0xb4648f67 in nsTransferable::AddDataFlavor (this=0x98104360, aDataFlavor=0x9811f750 "text/x-moz-url") at /media/Projects/mozilla/fx-team/widget/xpwidgets/nsTransferable.cpp:446 #7 0xb4a94578 in NS_InvokeByIndex_P () from /media/Projects/mozilla/objdir-fx-team-patches/dist/bin/libxul.so #8 0xb41ef5e4 in CallMethodHelper::Invoke (this=0xbfdaed60) at /media/Projects/mozilla/fx-team/js/xpconnect/src/XPCWrappedNative.cpp:3081 #9 0xb41ed520 in CallMethodHelper::Call (this=0xbfdaed60) at /media/Projects/mozilla/fx-team/js/xpconnect/src/XPCWrappedNative.cpp:2415 #10 0xb41ed3c6 in XPCWrappedNative::CallMethod (ccx=..., mode=XPCWrappedNative::CALL_METHOD) at /media/Projects/mozilla/fx-team/js/xpconnect/src/XPCWrappedNative.cpp:2381 #11 0xb41f9a52 in XPC_WN_CallMethod (cx=0x9833d6a0, argc=3, vp=0xac6902e8) at /media/Projects/mozilla/fx-team/js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1488 #12 0xb5604421 in js::CallJSNative (cx=0x9833d6a0, native=0xb41f9816 <XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*)>, args=...) at /media/Projects/mozilla/fx-team/js/src/jscntxtinlines.h:364 #13 0xb560d692 in js::InvokeKernel (cx=0x9833d6a0, args=..., construct=js::NO_CONSTRUCT) at /media/Projects/mozilla/fx-team/js/src/jsinterp.cpp:389 #14 0xb5615037 in js::Interpret (cx=0x9833d6a0, entryFrame=0xac690280, interpMode=js::JSINTERP_NORMAL) at /media/Projects/mozilla/fx-team/js/src/jsinterp.cpp:2348 #15 0xb560d1c9 in js::RunScript (cx=0x9833d6a0, script=..., fp=0xac690280) at /media/Projects/mozilla/fx-team/js/src/jsinterp.cpp:346 #16 0xb560d7d0 in js::InvokeKernel (cx=0x9833d6a0, args=..., construct=js::NO_CONSTRUCT) at /media/Projects/mozilla/fx-team/js/src/jsinterp.cpp:404 #17 0xb5615037 in js::Interpret (cx=0x9833d6a0, entryFrame=0xac690160, interpMode=js::JSINTERP_NORMAL) at /media/Projects/mozilla/fx-team/js/src/jsinterp.cpp:2348 #18 0xb560d1c9 in js::RunScript (cx=0x9833d6a0, script=..., fp=0xac690160) at /media/Projects/mozilla/fx-team/js/src/jsinterp.cpp:346 #19 0xb560d7d0 in js::InvokeKernel (cx=0x9833d6a0, args=..., construct=js::NO_CONSTRUCT) at /media/Projects/mozilla/fx-team/js/src/jsinterp.cpp:404 #20 0xb555c95a in js::Invoke (cx=0x9833d6a0, args=..., construct=js::NO_CONSTRUCT) at /media/Projects/mozilla/fx-team/js/src/jsinterp.h:112 #21 0xb560da79 in js::Invoke (cx=0x9833d6a0, thisv=..., fval=..., argc=1, argv=0xbfdb08f8, rval=0xbfdb0720) at /media/Projects/mozilla/fx-team/js/src/jsinterp.cpp:437 #22 0xb554f532 in JS_CallFunctionValue (cx=0x9833d6a0, objArg=0xa220b550, fval=..., argc=1, argv=0xbfdb08f8, rval=0xbfdb0720) at /media/Projects/mozilla/fx-team/js/src/jsapi.cpp:5789 #23 0xb41e344b in nsXPCWrappedJSClass::CallMethod (this=0xa62f5550, wrapper=0xa6d2ac00, methodIndex=3, info_=0xaef314c8, nativeParams=0xbfdb0a38) at /media/Projects/mozilla/fx-team/js/xpconnect/src/XPCWrappedJSClass.cpp:1432 #24 0xb41d9397 in nsXPCWrappedJS::CallMethod (this=0xa6d2ac00, methodIndex=3, info=0xaef314c8, params=0xbfdb0a38) at /media/Projects/mozilla/fx-team/js/xpconnect/src/XPCWrappedJS.cpp:580 #25 0xb4a95233 in PrepareAndDispatch (methodIndex=3, self=0xab093660, args=0xbfdb0b04) at /media/Projects/mozilla/fx-team/xpcom/reflect/xptcall/src/md/unix/xptcstubs_gcc_x86_unix.cpp:60 #26 0xb4a94578 in NS_InvokeByIndex_P () from /media/Projects/mozilla/objdir-fx-team-patches/dist/bin/libxul.so #27 0xb41ef5e4 in CallMethodHelper::Invoke (this=0xbfdb0bf0) at /media/Projects/mozilla/fx-team/js/xpconnect/src/XPCWrappedNative.cpp:3081 #28 0xb41ed520 in CallMethodHelper::Call (this=0xbfdb0bf0) at /media/Projects/mozilla/fx-team/js/xpconnect/src/XPCWrappedNative.cpp:2415 #29 0xb41ed3c6 in XPCWrappedNative::CallMethod (ccx=..., mode=XPCWrappedNative::CALL_METHOD) at /media/Projects/mozilla/fx-team/js/xpconnect/src/XPCWrappedNative.cpp:2381 #30 0xb41f9a52 in XPC_WN_CallMethod (cx=0x9833d6a0, argc=1, vp=0xac690130) at /media/Projects/mozilla/fx-team/js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1488 #31 0xb5604421 in js::CallJSNative (cx=0x9833d6a0, native=0xb41f9816 <XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*)>, args=...) at /media/Projects/mozilla/fx-team/js/src/jscntxtinlines.h:364 #32 0xb58ba41b in js::mjit::CallCompiler::generateNativeStub (this=0xbfdb14d0) at /media/Projects/mozilla/fx-team/js/src/methodjit/MonoIC.cpp:1073 #33 0xb58bb6c3 in js::mjit::ic::NativeCall (f=..., ic=0xa5fce31c) at /media/Projects/mozilla/fx-team/js/src/methodjit/MonoIC.cpp:1326 #34 0xa7b50f86 in ?? () #35 0xb6c3eff4 in ?? () from /media/Projects/mozilla/objdir-fx-team-patches/dist/bin/libxul.so

Comment on attachment 691398 [details] [diff] [review] landable preffed-off patch Review of attachment 691398 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/places/content/downloadsView.js @@ +907,5 @@ > + > + _getURLFromClipboardData: function DPV__getURLFromClipboardData() { > + let trans = Cc["@mozilla.org/widget/transferable;1"]. > + createInstance(Ci.nsITransferable); > + let flavors = ["text/x-moz-url", "text/unicode"]; You need to initialize the nsITransferable, or else debug builds will fail an assertion and crash. I think trans.init(null); should suffice.

Still pref-ed off, or course. I've not addressed the comments about private-browsing+save-as. The code there is mostly a stub and I'll rethink it anyway in the coming days. This is ready to land.

I was going to check this in, but inbound is closed now.

Just a note that with this patch, I'm getting pretty awful performance on a debug build on Linux. Clicking on the Downloads item in Places takes ~5 seconds to render the download list. Here's a profile: http://people.mozilla.com/~bgirard/cleopatra/#report=719d46c8521ee7e888c4f8b2aae5e941864cebb4 We'll need to investigate this in a follow-up once this patch lands.

I suspect the various file operations, and many downloads. btw we can fix performances later.

Thanks Eshan. Please do not close this bug after landing this on central. Regarding performance, there's quite a lot we can do. I'll detail that later.

Every single bc run since this push has been orange: TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_410196_paste_into_tags.js | uncaught exception - TypeError: PlacesOrganizer._content is undefined at chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_410196_paste_into_tags.js:104 etc. An example log is at https://tbpl.mozilla.org/php/getParsedLog.php?id=17950499&tree=Mozilla-Inbound Backed out from inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/b6e550165361

Mano: It looks like those tests relied on _content, which you removed. Can you give them a substitute? -Mike

Comment on attachment 692272 [details] [diff] [review] Fix the copy menuitem I broke Review of attachment 692272 [details] [diff] [review]: ----------------------------------------------------------------- So, one problem I've noticed with this patch, is that the onContentTreeSelect function was removed, but there are still plenty of places that still call it in the code. What should we be replacing that with instead? updateDetailsPane?

So, this patch, after applied to attachment 692272 [details] [diff] [review], should fix the tests that involve _content. There are still a bunch of tests failing though - http://www.pastebin.mozilla.org/1998100

Comment on attachment 692634 [details] [diff] [review] Fixes tests that rely on PlacesOrganizer._content This looks ok. I'll land it along with my patch.

Comment on attachment 692889 [details] [diff] [review] Don't reload the view multiple times Review of attachment 692889 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/places/content/downloadsView.js @@ +554,5 @@ > + // the places setter. > + let downloadsData = DownloadsCommon.getData(window.opener || window); > + downloadsData.addView(this); > + > + this._searchTerm = ""; Move this searchTerms init before getting downloadsData, so the "unload" listener follows the addView call (since they are clearly related). Or you may not init it and make the getter return this._searchTerm || ""; @@ +665,5 @@ > else { > this._richlistbox.appendChild(newOrUpdatedShell.element); > } > + > + if (this.searchTerm != "") { just if (this.searchTerm)

After landing the last attachment, I'd like this to be resolved and follow-ups to be filed, even just for clenaups or whatever else. let's not scope creep too much here.

How can we enable it on Nightly ?

(In reply to Guillaume C. [:ge3k0s] from comment #90) > How can we enable it on Nightly ? You must add a boolean pref with the name "browser.library.useNewDownloadsView", and set it to true.

(In reply to Mike Conley (:mconley) from comment #91) > (In reply to Guillaume C. [:ge3k0s] from comment #90) > > How can we enable it on Nightly ? > > You must add a boolean pref with the name > "browser.library.useNewDownloadsView", and set it to true. I added this pref in latest hourly m-c win32 build and there are constant slow-script warnings when opening the library and clicking on 'downloads' Is this a known issue ? cset tested: https://hg.mozilla.org/mozilla-central/rev/ba26dc1c6267 win7 x64 win32 m-c build (hourly)

Yes, there is much to optimize here, but we'll be doing that in follow-up bugs. See comment 75, comment 77 and comment 78.

Comment on attachment 692889 [details] [diff] [review] Don't reload the view multiple times http://hg.mozilla.org/integration/mozilla-inbound/rev/a09a149d8a44

as I said, let's consider this resolved at the next merge. please file follow-up bugs for any additional cleanups and issues.