Issue 5230 - Properly download Chromium on macOS

chromium_process.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2017 eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 /* eslint-env node */
 /* eslint no-console: "off" */
 
 "use strict";
 
 const childProcess = require("child_process");
 const fs = require("fs");
 const https = require("https");
 const os = require("os");
 const path = require("path");
-const process = require("process");

[Wladimir Palant, 2017/05/18 11:18:27]

You don't need this require, process is a global variable for all Node modules
(see https://nodejs.org/api/globals.html#globals_process).

[hub, 2017/05/18 13:22:37]

Acknowledged.

 
 const remoteInterface = require("chrome-remote-interface");
 const extractZip = require("extract-zip");
 
 const CHROMIUM_REVISION = 467222;
 
 function rmdir(dirPath)
 {
   for (let file of fs.readdirSync(dirPath))
   {
@@ skipping 49 matching lines @@
     "darwin": ["Mac", "chrome-mac.zip"]
   };
 
   if (!buildTypes.hasOwnProperty(platform))
   {
     let err = new Error(`Cannot run browser tests, ${platform} is unsupported`);
     return Promise.reject(err);
   }
 
 
-  return new Promise((resolve, reject) =>

[Wladimir Palant, 2017/05/18 11:18:28]

There is no async action here that isn't based on promises already, no point
using the Promise constructor. You can achieve the same effect like this:

  return Promise.resolve().then(() =>
  {
    ...
    if (fs.existsSync(chromiumDir))
      return chromiumDir;
    ...
    return download().then(unzip).then(() => chromiumDir);
  }).then(dir => getChromiumExecutable(dir));

[hub, 2017/05/18 13:22:37]

Done.

-  {
-    const snapshotsDir = path.join(__dirname, "chromium-snapshots");
-    const chromiumDir = path.join(snapshotsDir,
+  return Promise.resolve().then(() =>
+  {
+    let snapshotsDir = path.join(__dirname, "chromium-snapshots");
+    let chromiumDir = path.join(snapshotsDir,
                                 `chromium-${platform}-${CHROMIUM_REVISION}`);
     if (fs.existsSync(chromiumDir))
-      resolve(chromiumDir);
-    else
-    {
-      if (!fs.existsSync(path.dirname(chromiumDir)))
-        fs.mkdirSync(path.dirname(chromiumDir));
-
-      let [dir, fileName] = buildTypes[platform];
-      const archive = path.join(snapshotsDir, "download-cache",
-                              `${CHROMIUM_REVISION}-${fileName}`);
-      const url = `https://www.googleapis.com/download/storage/v1/b/chromium-browser-snapshots/o/${dir}%2F${CHROMIUM_REVISION}%2F${fileName}?alt=media`;

[Wladimir Palant, 2017/05/18 11:18:28]

You changed a bunch more variables from let to const here... Our coding style
actually mentions this explicitly under
https://adblockplus.org/en/coding-style#javascript-modern:
That's not what we have here so please change it back.

Note that we discussed that rule on IRC today (particularly given that there are
multiple opinions here) and decided that changing it isn't feasible/worthwhile
right now.

[hub, 2017/05/18 13:22:37]

I ended up using const like in C++ which is not exactly what it means in ES6.
Will change back. Sorry about that.

-
-      downloadCache(url, archive)
-        .then(() => unzipArchive(archive, chromiumDir))
-        .then(() => resolve(chromiumDir));
-    }
+      return chromiumDir;
+
+    if (!fs.existsSync(path.dirname(chromiumDir)))
+      fs.mkdirSync(path.dirname(chromiumDir));
+
+    let [dir, fileName] = buildTypes[platform];
+    let archive = path.join(snapshotsDir, "download-cache",
+                            `${CHROMIUM_REVISION}-${fileName}`);
+
+    return Promise.resolve()
+      .then(() =>
+      {
+        if (!fs.existsSync(archive))
+        {
+          let url = `https://www.googleapis.com/download/storage/v1/b/chromium-browser-snapshots/o/${dir}%2F${CHROMIUM_REVISION}%2F${fileName}?alt=media`;
+          console.info("Downloading Chromium...");
+          return download(url, archive);
+        }
+        console.info(`Reusing cached archive ${archive}`);
+      })
+      .then(() => unzipArchive(archive, chromiumDir))
+      .then(() => chromiumDir);
   }).then(dir => getChromiumExecutable(dir));
 }
 
-function downloadCache(url, destArchive)

[Wladimir Palant, 2017/05/18 11:18:27]

Nit: the function name sounds strange, downloadCached maybe?

More substantial issue: for what should be a generic function, this one has too
much application logic in it. It starts with the parameter name destArchive -
there is no reason for this particular function to assume that the download is
an archive. And it continues with the console.info() output which also mentions
archives and Chromium. Maybe handling the cache in ensureChromium() and keeping
this a completely generic download() helper like I suggested makes more sense
after all?

If you dislike the code I suggested, the following pattern would work as well:

  return Promise.resolve()
    .then(() =>
    {
      if (!fs.existsSync(archive))
      {
        console.info("Downloading Chromium...");
        return download(url, achive);
      }

      console.info(`Reusing cached archive ${archive}`);
    })
    .then(() => unzipArchive(archive, chromiumDir))
    .then(() => resolve(chromiumDir));

[hub, 2017/05/18 13:22:37]

fixing it.

+function download(url, destFile)
 {
   return new Promise((resolve, reject) =>
   {
-    if (fs.existsSync(destArchive))
-    {
-      console.info(`Reusing cached archive ${destArchive}`);
-      resolve();
-    }
-    else
-    {
-      console.info("Downloading Chromium...");
-      let cacheDir = path.dirname(destArchive);
-      if (!fs.existsSync(cacheDir))
-        fs.mkdirSync(cacheDir);
-      let tempDest = destArchive + "-" + process.pid;
-      let writable = fs.createWriteStream(tempDest);
-
-      https.get(url, response =>
-      {
-        if (response.statusCode != 200)
-        {
-          reject(new Error(`Unexpected server response: ${response.statusCode}`));

[Wladimir Palant, 2017/05/18 11:18:28]

I'm pretty sure that ESLint dislikes the length of that line.

[hub, 2017/05/18 13:22:37]

it does. Fixing.

-          response.resume();
-          return;
-        }
-
-        response.pipe(writable)
-                .on("error", error =>
-                {
-                  fs.unlinkSync(tempDest);
-                  reject(error);
-                })
-                .on("close", () =>
-                {
-                  writable.close();
-                  fs.renameSync(tempDest, destArchive);
-                  resolve();
-                });
-      }).on("error", reject);
-    }
+    let cacheDir = path.dirname(destFile);
+    if (!fs.existsSync(cacheDir))
+      fs.mkdirSync(cacheDir);
+    let tempDest = destFile + "-" + process.pid;
+    let writable = fs.createWriteStream(tempDest);
+
+    https.get(url, response =>
+    {
+      if (response.statusCode != 200)
+      {
+        reject(
+          new Error(`Unexpected server response: ${response.statusCode}`));
+        response.resume();
+        return;
+      }
+
+      response.pipe(writable)
+              .on("error", error =>
+              {
+                writable.close();
+                fs.unlinkSync(tempDest);
+                reject(error);
+              })
+              .on("close", () =>
+              {
+                writable.close();
+                fs.renameSync(tempDest, destFile);
+                resolve();
+              });
+    }).on("error", reject);
   });
 }
 
 function unzipArchive(archive, destDir)
 {
   return new Promise((resolve, reject) =>
   {
     extractZip(archive, {dir: destDir}, err =>
     {
       if (err)
@@ skipping 146 matching lines @@
     {
       child.kill();
       return result;
     }).catch(error =>
     {
       child.kill();
       throw error;
     });
   });
 };