The operations executed when a download completes in nsDownloadManager.cpp by nsDownload::SetState should also be executed by the new downloads API.

This implements the download done platform specific operations. nsDownloadManager::SetState also does some other things that aren't implemented here: - launches/shows the application as this is bug 852482 - copies the temporary file to the final place - shows a download done alert - remove the download because of the retention behaviour This patch operates on download.target.file. I haven't fully tested this yet, as I need to write a test which uses a real final file.

Comment on attachment 782690 [details] [diff] [review] Download done, platform specific operations Looks like a good start, I've not made a detailed review yet. Maybe we can separate AddToRecentDocs into a separate function, so that we don't need the aIsPrivate argument? A unit test could then check that the function is not called on completion for private downloads. In the final version we should proxy the call through the DownloadIntegration module, like we are doing for the launch functions, so that we can disable them during regression tests.

> Maybe we can separate AddToRecentDocs into a separate function, so that we > don't need the aIsPrivate argument? A unit test could then check that the > function is not called on completion for private downloads. How would the test check this?

(In reply to Neil Deakin from comment #4) > > Maybe we can separate AddToRecentDocs into a separate function, so that we > > don't need the aIsPrivate argument? A unit test could then check that the > > function is not called on completion for private downloads. > > How would the test check this? Similarly to what we do with "test_launch" in bug 852482, we need to call a function in DownloadIntegration, and ensure that during regression tests the function doesn't actually call the XPCOM component but sets a test flag indicating that it was called.

Also, differently form "test_launch", the download should wait for the promise returned by the DownloadIntegration function to be resolved before declaring the download completed, so we don't have the issue with needing to set a deferred object in test mode, like we do in "test_launch".

> Maybe we can separate AddToRecentDocs into a separate function, so that we > don't need the aIsPrivate argument? A unit test could then check that the > function is not called on completion for private downloads. The code seems simpler as is. (In reply to Paolo Amadini [:paolo] from comment #5) > Similarly to what we do with "test_launch" in bug 852482, we need to call a > function in DownloadIntegration, and ensure that during regression tests the > function doesn't actually call the XPCOM component but sets a test flag > indicating that it was called. This seems quite silly. In this case I want the component to be called during tests. Adding some kid of test flag just adds more complexity and only serves to test the test flag code rather than the actual code.

Not sure if there's a concern over the synchronous operations happening in platform specific code here.

(In reply to Neil Deakin from comment #7) > (In reply to Paolo Amadini [:paolo] from comment #5) > > Similarly to what we do with "test_launch" in bug 852482, we need to call a > > function in DownloadIntegration, and ensure that during regression tests the > > function doesn't actually call the XPCOM component but sets a test flag > > indicating that it was called. > > This seems quite silly. In this case I want the component to be called > during tests. Adding some kid of test flag just adds more complexity and > only serves to test the test flag code rather than the actual code. I agree that, in this specific case, preventing the XPCOM code from being run may be overkill and reduce coverage. In fact, running the operations doesn't affect other tests. In any case, in test mode we should set a flag to indicate that the function has been called, and verify it, in addition to calling the original platform integration module. This gives coverage in case of code refactoring. (In reply to Neil Deakin from comment #8) > Not sure if there's a concern over the synchronous operations happening in > platform specific code here. There probably is, for the future, but I don't think we have an easy way to address this when porting the code. At least, this is not a regression.

Comment on attachment 785063 [details] [diff] [review] downloaddone Review of attachment 785063 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, I've only rapidly inspected the C++ code, feel free to ask someone more expert in C++ components if you want. ::: toolkit/components/jsdownloads/src/DownloadIntegration.jsm @@ +64,5 @@ > createBundle("chrome://mozapps/locale/downloads/downloads.properties"); > }); > > +const gDownloadPlatform = Cc["@mozilla.org/toolkit/download-platform;1"] > + .getService(Ci.mozIDownloadPlatform); Lazy getter please (we're consistently using them here). @@ +295,5 @@ > + * The download object. > + * > + * @return none > + */ > + downloadDone: function(aDownload) { This should return a Promise like the other DownloadIntegration functions, as any part of the implementation may become asynchronous at any time. For now it could just be: try { // gDownloadPlatform... return Promise.resolve(); } catch (ex) { return Promise.reject(ex); } ::: toolkit/components/jsdownloads/src/DownloadPlatform.cpp @@ +74,5 @@ > + * Add the download to the recent documents list > + * Save the source uri in the downloaded file's metadata > + * Android: > + * Scan media > + */ This documentation should be moved to the IDL. ::: toolkit/components/jsdownloads/src/DownloadPlatform.h @@ +28,5 @@ > +protected: > + > + nsresult ExecuteDesiredAction(nsIFile* aTempFile, nsIFile* aDestination, bool aIsPrivate); > + nsresult MoveTempToTarget(nsIFile* aTempFile, nsIFile* aDestination); > + nsresult OpenWithApplication(nsIFile* aTempFile, nsIFile* aDestination, bool aIsPrivate); Is this patch a preliminary untested version? I don't see these implemented anywhere, and I don't see how this can link correctly. ::: toolkit/components/jsdownloads/src/moz.build @@ +8,5 @@ > + > +XPIDL_SOURCES += [ > + 'mozIDownloadPlatform.idl', > +] > + nit: whitespace ::: toolkit/components/jsdownloads/test/unit/test_DownloadIntegration.js @@ +136,5 @@ > add_task(function test_getTemporaryDownloadsDirectory() > { > let downloadDir = yield DownloadIntegration.getTemporaryDownloadsDirectory(); > do_check_true(downloadDir instanceof Ci.nsIFile); > + nit: whitespace @@ +160,5 @@ > + > + let download = yield Downloads.createDownload({ > + source: httpUrl("source.txt"), > + target: downloadFile, > + }); This should probably be located in common_test_Download.js and use promiseStartDownload/promiseDownloadStopped for full DownloadLegacy coverage, in addition to testing the result. @@ +168,5 @@ > + > +/** > + * This test is similar to the previous one except that it checks a private download. > + */ > +add_task(function test_platform_integration_private() You may also do a single iterative test case with "for (let isPrivate of [false, true])" if you want. Also two test cases is fine.

Comment on attachment 785063 [details] [diff] [review] downloaddone Review of attachment 785063 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/jsdownloads/src/DownloadCore.jsm @@ +389,5 @@ > this._notifyChange(); > if (this.succeeded) { > this._deferSucceeded.resolve(); > > + DownloadIntegration.downloadDone(this); I just remembered that these completion operations should be done before _deferSucceeded is resolved, so we should call yield on the promise returned by downloadDone before calling "resolve". This is because the resolution of _deferSucceeded means that the file is ready in its final state, including any metadata required by the platform. I think we should have a test that registers a handler on the whenSucceded() promise and verifies that the DownloadIntegration function has already been called when the handler is invoked.

Comment on attachment 786424 [details] [diff] [review] downloaddone I'll ask Marco to review the C++ parts.

Comment on attachment 786424 [details] [diff] [review] downloaddone Review of attachment 786424 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/jsdownloads/src/DownloadIntegration.jsm @@ +295,5 @@ > + * aParam aDownload > + * The download object. > + * > + * @return {Promise} > + * @return none @param aDownload The Download object. @return {Promise} @resolves When all the operations completed successfully. @rejects JavaScript exception if any of the operations failed. ::: toolkit/components/jsdownloads/src/DownloadPlatform.cpp @@ +20,5 @@ > +#ifdef XP_MACOSX > +#include <CoreFoundation/CoreFoundation.h> > +#endif > + > +#ifdef MOZ_WIDGET_ANDROID Is MOZ_WIDGET_ANDROID now replaced by ANDROID? We use the latter in the DownloadIntegration module. ::: toolkit/components/jsdownloads/src/DownloadPlatform.h @@ +8,5 @@ > +#include "mozIDownloadPlatform.h" > + > +#include "nsCOMPtr.h" > +#include "nsIURI.h" > +#include "nsIFile.h" nit: I seem to remember we prefer forward class declarations where possible in the header file, with includes in the CPP file. Not sure if this applies here. ::: toolkit/components/jsdownloads/src/mozIDownloadPlatform.idl @@ +23,5 @@ > + * Save the source uri in the downloaded file's metadata > + * Android: > + * Scan media > + * > + * @param aSource source uri of the download style nit: @param aSource Source URI of the download. ::: toolkit/components/jsdownloads/test/unit/common_test_Download.js @@ +1548,5 @@ > + DownloadIntegration.downloadDoneCalled = false; > + > + let downloadFile = yield DownloadIntegration.getSystemDownloadsDirectory(); > + downloadFile = downloadFile.clone(); > + downloadFile.append("test" + (Math.floor(Math.random() * 1000000))); nit: targetFile A comment on why we don't use the standard target file in the temporary directory is welcome (I think this is to ensure we test setting the "searchable" attribute on Windows). @@ +1552,5 @@ > + downloadFile.append("test" + (Math.floor(Math.random() * 1000000))); > + > + let download; > + if (gUseLegacySaver) { > + download = yield promiseStartLegacyDownload(httpUrl("source.txt"), { targetFile: downloadFile }); indentation nit: download = yield promiseStartLegacyDownload(httpUrl("source.txt"), { targetFile: targetFile }); @@ +1570,5 @@ > + do_check_true(DownloadIntegration.downloadDoneCalled); > + }); > + > + yield promiseDownloadStopped(download); > + do_check_true(whenSucceededCalled); We can't be sure of the order of resolution of different promises. The best approximation of the test can be done without whenSucceededCalled: // Wait for the whenSucceeded promise to be resolved first. yield download.whenSucceeded().then(function () { do_check_true(DownloadIntegration.downloadDoneCalled); }); // Then, wait for the promise returned by "start" to be resolved. yield promiseDownloadStopped(download);

> > +#ifdef MOZ_WIDGET_ANDROID > > Is MOZ_WIDGET_ANDROID now replaced by ANDROID? We use the latter in the > DownloadIntegration module. I actually just copied this from nsDownloadManager.cpp. The call made here is to AndroidBridge::ScanMedia, which is defined in widget/android, so using MOZ_WIDGET_ANDROID is correct here. The one in DownloadIntegration isn't dependent on the widget code so it should be fine as is as well.

Comment on attachment 786424 [details] [diff] [review] downloaddone Review of attachment 786424 [details] [diff] [review]: ----------------------------------------------------------------- ::: b2g/installer/package-manifest.in @@ +239,5 @@ > @BINPATH@/components/imgicon.xpt > @BINPATH@/components/inspector.xpt > @BINPATH@/components/intl.xpt > @BINPATH@/components/jar.xpt > +@BINPATH@/components/jsdownloads.xpt It looks like this shouldn't be a replacement. ::: toolkit/components/jsdownloads/src/DownloadPlatform.cpp @@ +13,5 @@ > +#include <shlobj.h> > +#include "nsILocalFileWin.h" > +#ifdef DOWNLOAD_SCANNER > +#include "nsDownloadScanner.h" > +#endif Unused #include "nsDownloadScanner.h" @@ +41,5 @@ > + > + NS_ADDREF(gDownloadPlatformService); > + > +#if defined(MOZ_WIDGET_GTK2) > + g_type_init(); indentation nit @@ +74,5 @@ > + { > + bool addToRecentDocs = true; > + nsCOMPtr<nsIPrefBranch> pref(do_GetService(NS_PREFSERVICE_CONTRACTID)); > + if (pref) > + pref->GetBoolPref(PREF_BDM_ADDTORECENTDOCS, &addToRecentDocs); nit: brace single line blocks, may also use "Preferences::" @@ +127,5 @@ > + > +#ifdef XP_WIN > + // Adjust file attributes so that by default, new files are indexed > + // by desktop search services. Skip off those that land in the temp > + // folder. nit: comment may be better indented @@ +135,5 @@ > + aTarget->GetParent(getter_AddRefs(fileDir)); > + > + bool isTemp = false; > + if (fileDir) > + fileDir->Equals(tempDir, &isTemp); nit: brace single line blocks @@ +139,5 @@ > + fileDir->Equals(tempDir, &isTemp); > + > + nsCOMPtr<nsILocalFileWin> localFileWin(do_QueryInterface(aTarget)); > + if (!isTemp && localFileWin) > + localFileWin->SetFileAttributesWin(nsILocalFileWin::WFA_SEARCH_INDEXED); ditto ::: toolkit/components/jsdownloads/src/moz.build @@ +6,5 @@ > > +MODULE = 'jsdownloads' > + > +XPIDL_SOURCES += [ > + 'mozIDownloadPlatform.idl', I think this should go in the "jsdownloads/public" folder, unless putting it in the "src" folder here makes it private to the module. ::: toolkit/components/jsdownloads/test/unit/test_DownloadIntegration.js @@ -145,5 @@ > let tempDir = Services.dirsvc.get("TmpD", Ci.nsIFile); > do_check_eq(downloadDir.path, tempDir.path); > } > }); > - Is this file touched intentionally?

I had to make a few changes for this to build locally. Since we're adding a new makefile, this requires a Build System review. Gregory, is this the recommended way for adding a C++ XPCOM component?

Comment on attachment 795058 [details] [diff] [review] Updated to build locally Review of attachment 795058 [details] [diff] [review]: ----------------------------------------------------------------- Covers just the build bits.

Neil, do you think the latest patch is ready to land, or does this bug require some more manual testing?

Marking as [qa-] based on the in-testsuite flag and on the existing unit test in the pushlog