fix: fall back to default logs path in getPath('logs') (#19653)

docs/api/app.md

@@ -582,7 +582,7 @@ them.
 
 Sets or creates a directory your app's logs which can then be manipulated with `app.getPath()` or `app.setPath(pathName, newPath)`.
 
-Calling `app.setAppLogsPath()` without a `path` parameter will result in this directory being set to `/Library/Logs/YourAppName` on _macOS_, and inside the `userData` directory on _Linux_ and _Windows_.
+Calling `app.setAppLogsPath()` without a `path` parameter will result in this directory being set to `~/Library/Logs/YourAppName` on _macOS_, and inside the `userData` directory on _Linux_ and _Windows_.
 
 ### `app.getAppPath()`
 
@@ -614,6 +614,8 @@ Returns `String` - The current application directory.
 Returns `String` - A path to a special directory or file associated with `name`. On
 failure, an `Error` is thrown.
 
+If `app.getPath('logs')` is called without called `app.setAppLogsPath()` being called first, a default log directory will be created equivalent to calling `app.setAppLogsPath()` without a `path` parameter.
+
 ### `app.getFileIcon(path[, options])`
 
 * `path` String

native_mate/native_mate/arguments.h

@@ -9,6 +9,7 @@
 #include <vector>
 
 #include "base/macros.h"
+#include "base/optional.h"
 #include "native_mate/converter.h"
 
 namespace mate {
@@ -34,6 +35,17 @@ class Arguments {
     return ConvertFromV8(isolate_, info_->Data(), out);
   }
 
+  template <typename T>
+  bool GetNext(base::Optional<T>* out) {
+    if (next_ >= info_->Length())
+      return true;
+    v8::Local<v8::Value> val = (*info_)[next_];
+    bool success = ConvertFromV8(isolate_, val, out);
+    if (success)
+      next_++;
+    return success;
+  }
+
   template <typename T>
   bool GetNext(T* out) {
     if (next_ >= info_->Length()) {

shell/browser/api/atom_api_app.cc

@@ -11,6 +11,7 @@
 #include "base/environment.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
+#include "base/optional.h"
 #include "base/path_service.h"
 #include "base/system/sys_info.h"
 #include "chrome/browser/browser_process.h"
@@ -845,14 +846,14 @@ void App::SetAppPath(const base::FilePath& app_path) {
 }
 
 #if !defined(OS_MACOSX)
-void App::SetAppLogsPath(mate::Arguments* args) {
+void App::SetAppLogsPath(base::Optional<base::FilePath> custom_path,
-  base::FilePath custom_path;
+                         mate::Arguments* args) {
-  if (args->GetNext(&custom_path)) {
+  if (custom_path.has_value()) {
-    if (!custom_path.IsAbsolute()) {
+    if (!custom_path->IsAbsolute()) {
       args->ThrowError("Path must be absolute");
       return;
     }
-    base::PathService::Override(DIR_APP_LOGS, custom_path);
+    base::PathService::Override(DIR_APP_LOGS, custom_path.value());
   } else {
     base::FilePath path;
     if (base::PathService::Get(DIR_USER_DATA, &path)) {
@@ -867,17 +868,21 @@ void App::SetAppLogsPath(mate::Arguments* args) {
 base::FilePath App::GetPath(mate::Arguments* args, const std::string& name) {
   bool succeed = false;
   base::FilePath path;
+
   int key = GetPathConstant(name);
-  if (key >= 0)
+  if (key >= 0) {
     succeed = base::PathService::Get(key, &path);
-  if (!succeed) {
+    // If users try to get the logs path before setting a logs path,
-    if (name == "logs") {
+    // set the path to a sensible default and then try to get it again
-      args->ThrowError("Failed to get '" + name +
+    if (!succeed && name == "logs") {
-                       "' path: setAppLogsPath() must be called first.");
+      base::ThreadRestrictions::ScopedAllowIO allow_io;
-    } else {
+      SetAppLogsPath(base::Optional<base::FilePath>(), args);
+      succeed = base::PathService::Get(key, &path);
+    }
+  }
+
+  if (!succeed)
     args->ThrowError("Failed to get '" + name + "' path");
-    }
-  }
 
   return path;
 }

shell/browser/api/atom_api_app.h

@@ -164,7 +164,8 @@ class App : public AtomBrowserClient::Delegate,
   void ChildProcessLaunched(int process_type, base::ProcessHandle handle);
   void ChildProcessDisconnected(base::ProcessId pid);
 
-  void SetAppLogsPath(mate::Arguments* args);
+  void SetAppLogsPath(base::Optional<base::FilePath> custom_path,
+                      mate::Arguments* args);
 
   // Get/Set the pre-defined path in PathService.
   base::FilePath GetPath(mate::Arguments* args, const std::string& name);

shell/browser/api/atom_api_app_mac.mm

@@ -13,14 +13,14 @@ namespace electron {
 
 namespace api {
 
-void App::SetAppLogsPath(mate::Arguments* args) {
+void App::SetAppLogsPath(base::Optional<base::FilePath> custom_path,
-  base::FilePath custom_path;
+                         mate::Arguments* args) {
-  if (args->GetNext(&custom_path)) {
+  if (custom_path.has_value()) {
-    if (!custom_path.IsAbsolute()) {
+    if (!custom_path->IsAbsolute()) {
       args->ThrowError("Path must be absolute");
       return;
     }
-    base::PathService::Override(DIR_APP_LOGS, custom_path);
+    base::PathService::Override(DIR_APP_LOGS, custom_path.value());
   } else {
     NSString* bundle_name =
         [[[NSBundle mainBundle] infoDictionary] objectForKey:@"CFBundleName"];

shell/common/native_mate_converters/value_converter.h

@@ -7,6 +7,8 @@
 
 #include "native_mate/converter.h"
 
+#include "base/optional.h"
+
 namespace base {
 class DictionaryValue;
 class ListValue;
@@ -33,6 +35,29 @@ struct Converter<base::Value> {
                                    const base::Value& val);
 };
 
+template <typename T>
+struct Converter<base::Optional<T>> {
+  static bool FromV8(v8::Isolate* isolate,
+                     v8::Local<v8::Value> val,
+                     base::Optional<T>* out) {
+    if (val->IsNull() || val->IsUndefined()) {
+      return true;
+    }
+    T converted;
+    if (Converter<T>::FromV8(isolate, val, &converted)) {
+      return true;
+    }
+    out->emplace(converted);
+    return true;
+  }
+  static v8::Local<v8::Value> ToV8(v8::Isolate* isolate,
+                                   const base::Optional<T>& val) {
+    if (val.has_value())
+      return Converter<T>::ToV8(val.value());
+    return v8::Undefined(isolate);
+  }
+};
+
 template <>
 struct Converter<base::ListValue> {
   static bool FromV8(v8::Isolate* isolate,

spec-main/api-app-spec.ts

@@ -5,6 +5,7 @@ import * as https from 'https'
 import * as net from 'net'
 import * as fs from 'fs'
 import * as path from 'path'
+import { homedir } from 'os'
 import split = require('split')
 import { app, BrowserWindow, Menu } from 'electron'
 import { emittedOnce } from './events-helpers';
@@ -671,6 +672,30 @@ describe('app module', () => {
     })
   })
 
+  describe('getPath("logs")', () => {
+    const logsPaths = {
+      'darwin': path.resolve(homedir(), 'Library', 'Logs'),
+      'linux': path.resolve(homedir(), 'AppData', app.name),
+      'win32': path.resolve(homedir(), 'AppData', app.name),
+    }
+
+    it('has no logs directory by default', () => {
+      // this won't be deterministic except on CI since
+      // users may or may not have this dir
+      if (!isCI) return
+
+      const osLogPath = (logsPaths as any)[process.platform]
+      expect(fs.existsSync(osLogPath)).to.be.false
+    })
+
+    it('creates a new logs directory if one does not exist', () => {
+      expect(() => { app.getPath('logs') }).to.not.throw()
+
+      const osLogPath = (logsPaths as any)[process.platform]
+      expect(fs.existsSync(osLogPath)).to.be.true
+    })
+  })
+
   describe('getPath(name)', () => {
     it('returns paths that exist', () => {
       const paths = [