Refactor download handling so it works with node removal from window …

…renderer

Auditors: @bsclifton

Fix #5947

app/browser/electronDownloadItem.js

@@ -0,0 +1,27 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * 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/. */
+
+const downloadStates = require('../../js/constants/downloadStates')
+
+/**
+ * Maps downloadId to an electron download-item
+ */
+const downloadMap = {}
+
+module.exports.updateElectronDownloadItem = (downloadId, item, state) => {
+  if (state === downloadStates.INTERRUPTED || state === downloadStates.CANCELLED || state === downloadStates.COMPLETED) {
+    delete downloadMap[downloadId]
+  } else {
+    downloadMap[downloadId] = item
+  }
+}
+
+module.exports.cancelDownload = (downloadId) =>
+  downloadMap[downloadId] && downloadMap[downloadId].cancel()
+
+module.exports.pauseDownload = (downloadId) =>
+  downloadMap[downloadId] && downloadMap[downloadId].pause()
+
+module.exports.resumeDownload = (downloadId) =>
+  downloadMap[downloadId] && downloadMap[downloadId].resume()

app/browser/reducers/downloadsReducer.js

@@ -0,0 +1,96 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * 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/. */
+
+'use strict'
+
+const appConstants = require('../../../js/constants/appConstants')
+const downloadStates = require('../../../js/constants/downloadStates')
+const {clipboard, BrowserWindow, shell} = require('electron')
+const fs = require('fs')
+const path = require('path')
+const {cancelDownload, pauseDownload, resumeDownload} = require('../electronDownloadItem')
+const {CANCEL, PAUSE, RESUME} = require('../../common/constants/electronDownloadItemActions')
+
+const downloadsReducer = (state, action) => {
+  const download = action.downloadId ? state.getIn(['downloads', action.downloadId]) : undefined
+  if (!download &&
+      ![appConstants.APP_MERGE_DOWNLOAD_DETAIL,
+        appConstants.APP_CLEAR_COMPLETED_DOWNLOADS].includes(action.actionType)) {
+    return state
+  }
+  switch (action.actionType) {
+    case appConstants.APP_DOWNLOAD_REVEALED:
+      fs.exists(download.get('savePath'), (exists) => {
+        if (exists) {
+          shell.showItemInFolder(download.get('savePath'))
+        } else {
+          shell.openItem(path.dirname(download.get('savePath')))
+        }
+      })
+      break
+    case appConstants.APP_DOWNLOAD_OPENED:
+      fs.exists(download.get('savePath'), (exists) => {
+        if (exists) {
+          shell.openItem(download.get('savePath'))
+        } else {
+          shell.beep()
+        }
+      })
+      break
+    case appConstants.APP_DOWNLOAD_ACTION_PERFORMED:
+      switch (action.downloadAction) {
+        case CANCEL:
+          // It's important to update state before the cancel since it'll remove the reference
+          state = state.setIn(['downloads', action.downloadId, 'state'], downloadStates.CANCELLED)
+          cancelDownload(action.downloadId)
+          break
+        case PAUSE:
+          pauseDownload(action.downloadId)
+          state = state.setIn(['downloads', action.downloadId, 'state'], downloadStates.PAUSED)
+          break
+        case RESUME:
+          resumeDownload(action.downloadId)
+          state = state.setIn(['downloads', action.downloadId, 'state'], downloadStates.IN_PROGRESS)
+          break
+      }
+      break
+    case appConstants.APP_DOWNLOAD_COPIED_TO_CLIPBOARD:
+      clipboard.writeText(download.get('url'))
+      break
+    case appConstants.APP_DOWNLOAD_DELETED:
+      shell.moveItemToTrash(download.get('savePath'))
+      state = state.deleteIn(['downloads', action.downloadId])
+      break
+    case appConstants.APP_DOWNLOAD_CLEARED:
+      state = state.deleteIn(['downloads', action.downloadId])
+      break
+    case appConstants.APP_DOWNLOAD_REDOWNLOADED:
+      const win = BrowserWindow.getFocusedWindow()
+      if (win) {
+        win.webContents.downloadURL(download.get('url'))
+        state = state.deleteIn(['downloads', action.downloadId])
+      } else {
+        shell.beep()
+      }
+      break
+    case appConstants.APP_MERGE_DOWNLOAD_DETAIL:
+      if (action.downloadDetail) {
+        state = state.mergeIn(['downloads', action.downloadId], action.downloadDetail)
+      } else {
+        state = state.deleteIn(['downloads', action.downloadId])
+      }
+      break
+    case appConstants.APP_CLEAR_COMPLETED_DOWNLOADS:
+      if (state.get('downloads')) {
+        const downloads = state.get('downloads')
+          .filter((download) =>
+            ![downloadStates.COMPLETED, downloadStates.INTERRUPTED, downloadStates.CANCELLED].includes(download.get('state')))
+        state = state.set('downloads', downloads)
+      }
+      break
+  }
+  return state
+}
+
+module.exports = downloadsReducer

js/constants/downloadActions.js → app/common/constants/electronDownloadItemActions.js

@@ -2,13 +2,13 @@
  * 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/. */
 
-const mapValuesByKeys = require('../lib/functional').mapValuesByKeys
+const mapValuesByKeys = require('../../../js/lib/functional').mapValuesByKeys
 
 const _ = null
-const downloadActions = {
+const electronDownloadItemActions = {
   PAUSE: _,
   RESUME: _,
   CANCEL: _
 }
 
-module.exports = mapValuesByKeys(downloadActions)
+module.exports = mapValuesByKeys(electronDownloadItemActions)

app/filtering.js

@@ -12,7 +12,6 @@ const webContents = electron.webContents
 const appActions = require('../js/actions/appActions')
 const appConfig = require('../js/constants/appConfig')
 const downloadStates = require('../js/constants/downloadStates')
-const downloadActions = require('../js/constants/downloadActions')
 const urlParse = require('url').parse
 const getBaseDomain = require('../js/lib/baseDomain').getBaseDomain
 const getSetting = require('../js/settings').getSetting
@@ -31,6 +30,7 @@ const uuid = require('node-uuid')
 const path = require('path')
 const getOrigin = require('../js/state/siteUtil').getOrigin
 const {adBlockResourceName} = require('./adBlock')
+const {updateElectronDownloadItem} = require('./browser/electronDownloadItem')
 
 let appStore = null
 
@@ -46,11 +46,6 @@ const pdfjsOrigin = `chrome-extension://${config.PDFJSExtensionId}`
 // Third party domains that require a valid referer to work
 const refererExceptions = ['use.typekit.net', 'cloud.typography.com']
 
-/**
- * Maps downloadId to an electron download-item
- */
-const downloadMap = {}
-
 /**
  * Maps partition name to the session object
  */
@@ -467,11 +462,7 @@ module.exports.isThirdPartyHost = (baseContextHost, testHost) => {
 }
 
 function updateDownloadState (downloadId, item, state) {
-  if (state === downloadStates.INTERRUPTED || state === downloadStates.CANCELLED || state === downloadStates.COMPLETED) {
-    delete downloadMap[downloadId]
-  } else {
-    downloadMap[downloadId] = item
-  }
+  updateElectronDownloadItem(downloadId, item, state)
 
   if (!item) {
     appActions.mergeDownloadDetail(downloadId, { state: downloadStates.INTERRUPTED })
@@ -602,29 +593,6 @@ module.exports.init = (state, action, store) => {
       e.returnValue = true
       return e.returnValue
     })
-    ipcMain.on(messages.DOWNLOAD_ACTION, (e, downloadId, action) => {
-      const item = downloadMap[downloadId]
-      switch (action) {
-        case downloadActions.CANCEL:
-          updateDownloadState(downloadId, item, downloadStates.CANCELLED)
-          if (item) {
-            item.cancel()
-          }
-          break
-        case downloadActions.PAUSE:
-          if (item) {
-            item.pause()
-          }
-          updateDownloadState(downloadId, item, downloadStates.PAUSED)
-          break
-        case downloadActions.RESUME:
-          if (item) {
-            item.resume()
-          }
-          updateDownloadState(downloadId, item, downloadStates.IN_PROGRESS)
-          break
-      }
-    })
     ipcMain.on(messages.NOTIFICATION_RESPONSE, (e, message, buttonIndex, persist) => {
       if (permissionCallbacks[message]) {
         permissionCallbacks[message](buttonIndex, persist)

app/index.js

@@ -52,7 +52,6 @@ const Importer = require('./importer')
 const messages = require('../js/constants/messages')
 const appConfig = require('../js/constants/appConfig')
 const appActions = require('../js/actions/appActions')
-const downloadActions = require('../js/actions/downloadActions')
 const SessionStore = require('./sessionStore')
 const AppStore = require('../js/stores/appStore')
 const PackageLoader = require('./package-loader')
@@ -490,10 +489,6 @@ app.on('ready', () => {
       electron.clipboard.writeText(text)
     })
 
-    ipcMain.on(messages.OPEN_DOWNLOAD_PATH, (e, download) => {
-      downloadActions.openDownloadPath(Immutable.fromJS(download))
-    })
-
     ipcMain.on(messages.CERT_ERROR_ACCEPTED, (event, url) => {
       try {
         let host = urlParse(url).host

docs/appActions.md

@@ -541,6 +541,85 @@ Dispatch a message to copy data URL to clipboard
 
 
 
+### shuttingDown() 
+
+Dispatches a message when the app is shutting down.
+
+
+
+### downloadRevealed(downloadId) 
+
+Dispatches a message when a download is being revealed.
+Typically this will open the download directory in finder / explorer and select the icon.
+
+**Parameters**
+
+**downloadId**: `string`, ID of the download being revealed
+
+
+
+### downloadOpened(downloadId) 
+
+Dispatches a message when a download is being opened.
+
+**Parameters**
+
+**downloadId**: `string`, ID of the download being opened
+
+
+
+### downloadActionPerformed(downloadId, downloadAction) 
+
+Dispatches a message when an electron download action is being performed (pause, resume, cancel)
+
+**Parameters**
+
+**downloadId**: `string`, ID of the download item the action is being performed to
+
+**downloadAction**: `string`, the action to perform from constants/electronDownloadItemActions.js
+
+
+
+### downloadCopiedToClipboard(downloadId) 
+
+Dispatches a message when a download URL is being copied to the clipboard
+
+**Parameters**
+
+**downloadId**: `string`, ID of the download item being copied to the clipboard
+
+
+
+### downloadDeleted(downloadId) 
+
+Dispatches a message when a download is being deleted
+
+**Parameters**
+
+**downloadId**: `string`, ID of the download item being deleted
+
+
+
+### downloadCleared(downloadId) 
+
+Dispatches a message when a download is being cleared
+
+**Parameters**
+
+**downloadId**: `string`, ID of the download item being cleared
+
+
+
+### downloadRedownloaded(downloadId) 
+
+Dispatches a message when a download is being redownloaded
+
+**Parameters**
+
+**downloadId**: `string`, ID of the download item being redownloaded
+
+
+
 
 * * *
 

js/about/aboutActions.js

@@ -154,8 +154,11 @@ const aboutActions = {
     })
   },
 
-  openDownloadPath: function (download) {
-    ipc.send(messages.OPEN_DOWNLOAD_PATH, download.toJS())
+  downloadRevealed: function (downloadId) {
+    aboutActions.dispatchAction({
+      actionType: appConstants.APP_DOWNLOAD_REVEALED,
+      downloadId
+    })
   },
 
   decryptPassword: function (encryptedPassword, authTag, iv, id) {

js/about/downloads.js

@@ -29,7 +29,7 @@ class DownloadItem extends ImmutableComponent {
       className='listItem'
       onContextMenu={aboutActions.contextMenu.bind(this, contextMenuDownload, 'download')}
       data-context-menu-disable
-      onDoubleClick={aboutActions.openDownloadPath.bind(this, this.props.download)}>
+      onDoubleClick={aboutActions.downloadRevealed.bind(this, this.props.downloadId)}>
       {
         <div className='aboutListItem' title={this.props.download.get('url')}>
           <div className='aboutItemTitle'>{this.props.download.get('filename')}</div>