Fixing review comments

* Renamed the CLI arg to downloads-path
* Download is available only via 
GET /session/{sessionId}/se/file
* Files downloaded would be available ONLY as a zip file.

Author: krmahadevan

java/src/org/openqa/selenium/grid/node/Node.java

@@ -150,9 +150,6 @@ protected Node(Tracer tracer, NodeId id, URI uri, Secret registrationSecret) {
       post("/session/{sessionId}/se/file")
         .to(params -> new UploadFile(this, sessionIdFrom(params)))
         .with(spanDecorator("node.upload_file")),
-      get("/session/{sessionId}/file")
-        .to(params -> new DownloadFile(this, sessionIdFrom(params)))
-        .with(spanDecorator("node.download_file")),
       get("/session/{sessionId}/se/file")
         .to(params -> new DownloadFile(this, sessionIdFrom(params)))
         .with(spanDecorator("node.download_file")),

java/src/org/openqa/selenium/grid/node/config/NodeFlags.java

@@ -222,12 +222,12 @@ public class NodeFlags implements HasRoles {
   private String nodeImplementation = DEFAULT_NODE_IMPLEMENTATION;
 
   @Parameter(
-    names = {"--downloads-dir"},
+    names = {"--downloads-path"},
     description = "The default location wherein all browser triggered file downloads would be "
       + "available to be retrieved from. This is usually the directory that you configure in "
       + "your browser as the default location for storing downloaded files.")
-  @ConfigValue(section = NODE_SECTION, name = "downloads-dir", example = "")
-  private String downloadsDir = "";
+  @ConfigValue(section = NODE_SECTION, name = "downloads-path", example = "")
+  private String downloadsPath = "";
 
   @Override
   public Set<Role> getRoles() {

java/src/org/openqa/selenium/grid/node/config/NodeOptions.java

@@ -149,7 +149,7 @@ public Optional<URI> getPublicGridUri() {
   }
 
   public Optional<String> getDownloadsDirectory() {
-    return config.get(NODE_SECTION, "downloads-dir");
+    return config.get(NODE_SECTION, "downloads-path");
   }
 
   public Node getNode() {

java/src/org/openqa/selenium/grid/node/local/LocalNode.java

@@ -63,7 +63,6 @@
 import org.openqa.selenium.io.Zip;
 import org.openqa.selenium.json.Json;
 import org.openqa.selenium.remote.SessionId;
-import org.openqa.selenium.remote.http.Contents;
 import org.openqa.selenium.remote.http.HttpRequest;
 import org.openqa.selenium.remote.http.HttpResponse;
 import org.openqa.selenium.remote.tracing.AttributeKey;
@@ -123,7 +122,7 @@ public class LocalNode extends Node {
   private final int maxSessionCount;
   private final int configuredSessionCount;
   private final boolean cdpEnabled;
-  private final String downloadsDir;
+  private final String downloadsPath;
 
   private final boolean bidiEnabled;
   private final AtomicBoolean drainAfterSessions = new AtomicBoolean();
@@ -148,7 +147,7 @@ private LocalNode(
     Duration heartbeatPeriod,
     List<SessionSlot> factories,
     Secret registrationSecret,
-    String downloadsDir) {
+    String downloadsPath) {
     super(tracer, new NodeId(UUID.randomUUID()), uri, registrationSecret);
 
     this.bus = Require.nonNull("Event bus", bus);
@@ -165,7 +164,7 @@ private LocalNode(
     this.sessionCount.set(drainAfterSessionCount);
     this.cdpEnabled = cdpEnabled;
     this.bidiEnabled = bidiEnabled;
-    this.downloadsDir = Optional.ofNullable(downloadsDir).orElse("");
+    this.downloadsPath = Optional.ofNullable(downloadsPath).orElse("");
 
     this.healthCheck = healthCheck == null ?
                        () -> {
@@ -180,10 +179,8 @@ private LocalNode(
       .ticker(ticker)
       .removalListener((RemovalListener<SessionId, TemporaryFilesystem>) notification -> {
         TemporaryFilesystem tempFS = notification.getValue();
-        if (tempFS != null) {
-          tempFS.deleteTemporaryFiles();
-          tempFS.deleteBaseDir();
-        }
+        tempFS.deleteTemporaryFiles();
+        tempFS.deleteBaseDir();
       })
       .build();
 
@@ -485,17 +482,18 @@ public HttpResponse downloadFile(HttpRequest req, SessionId id) {
     if (slot != null && slot.getSession() instanceof DockerSession) {
       return executeWebDriverCommand(req);
     }
-    if (this.downloadsDir.isEmpty()) {
-      throw new WebDriverException(
-        "Please specify the directory that would contain downloaded files and restart the node.");
+    if (this.downloadsPath.isEmpty()) {
+      String msg = "Please specify the path wherein the files downloaded using the browser "
+        + "would be available via the command line arg [--downloads-path] and restart the node";
+      throw new WebDriverException(msg);
     }
-    File dir = new File(this.downloadsDir);
+    File dir = new File(this.downloadsPath);
     if (!dir.exists()) {
       throw new WebDriverException(
-        String.format("Cannot locate downloads directory %s.", downloadsDir));
+        String.format("Cannot locate downloads directory %s.", downloadsPath));
     }
     if (!dir.isDirectory()) {
-      throw new WebDriverException(String.format("Invalid directory: %s.",downloadsDir));
+      throw new WebDriverException(String.format("Invalid directory: %s.", downloadsPath));
     }
     String filename = req.getQueryParameter("filename");
     try {
@@ -504,13 +502,13 @@ public HttpResponse downloadFile(HttpRequest req, SessionId id) {
       ).orElse(new File[]{});
       if (allFiles.length == 0) {
         throw new WebDriverException(
-          String.format("Cannot find file [%s] in directory %s.", filename, downloadsDir));
+          String.format("Cannot find file [%s] in directory %s.", filename, downloadsPath));
       }
       if (allFiles.length != 1) {
         throw new WebDriverException(
           String.format("Expected there to be only 1 file. There were: %s.", allFiles.length));
       }
-      String content = Contents.string(allFiles[0]);
+      String content = Zip.zip(allFiles[0]);
       ImmutableMap<String, Object> result = ImmutableMap.of(
         "filename", filename,
         "contents", content);

java/test/org/openqa/selenium/grid/node/NodeTest.java

@@ -30,7 +30,7 @@
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 
-import java.util.Base64;
+import java.nio.file.Path;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.openqa.selenium.Capabilities;
@@ -495,15 +495,17 @@ void canDownloadAFile() throws IOException {
     Session session = response.right().getSession();
     String hello = "Hello, world!";
 
-    HttpRequest req = new HttpRequest(GET, String.format("/session/%s/file", session.getId()));
+    HttpRequest req = new HttpRequest(GET, String.format("/session/%s/se/file", session.getId()));
     File zip = createTmpFile(hello);
     req.addQueryParameter("filename", zip.getName());
     HttpResponse rsp = node.execute(req);
     node.stop(session.getId());
     Map<String, Object> map = new Json().toType(string(rsp), Json.MAP_TYPE);
     File baseDir = getTemporaryFilesystemBaseDir(TemporaryFilesystem.getDefaultTmpFS());
     String encodedContents = map.get("contents").toString();
-    String decodedContents = new String(Base64.getMimeDecoder().decode(encodedContents));
+    Zip.unzip(encodedContents, baseDir);
+    Path path = new File(baseDir.getAbsolutePath() + "/" + map.get("filename")).toPath();
+    String decodedContents = String.join("", Files.readAllLines(path));
     assertThat(decodedContents).isEqualTo(hello);
   }