Add a SessionPreferences to manage session related data

By design the BrowserClient should not be aware of the api:: classes.

atom/browser/api/atom_api_session.cc

@@ -17,6 +17,7 @@
 #include "atom/browser/atom_permission_manager.h"
 #include "atom/browser/browser.h"
 #include "atom/browser/net/atom_cert_verifier.h"
+#include "atom/browser/session_preferences.h"
 #include "atom/common/native_mate_converters/callback.h"
 #include "atom/common/native_mate_converters/content_converter.h"
 #include "atom/common/native_mate_converters/file_path_converter.h"
@@ -447,6 +448,8 @@ Session::Session(v8::Isolate* isolate, AtomBrowserContext* browser_context)
   content::BrowserContext::GetDownloadManager(browser_context)->
       AddObserver(this);
 
+  new SessionPreferences(browser_context);
+
   Init(isolate);
   AttachAsUserData(browser_context);
 }
@@ -680,18 +683,17 @@ void Session::CreateInterruptedDownload(const mate::Dictionary& options) {
       length, last_modified, etag, base::Time::FromDoubleT(start_time)));
 }
 
-void Session::AddPreload(const base::FilePath::StringType& preloadPath) {
-  preloads_.push_back(preloadPath);
-}
-
-void Session::RemovePreload(const base::FilePath::StringType& preloadPath) {
-  preloads_.erase(
-    std::remove(preloads_.begin(), preloads_.end(), preloadPath),
-    preloads_.end());
+void Session::SetPreloads(
+    const std::vector<base::FilePath::StringType>& preloads) {
+  auto* prefs = SessionPreferences::FromBrowserContext(browser_context());
+  DCHECK(prefs);
+  prefs->set_preloads(preloads);
 }
 
 std::vector<base::FilePath::StringType> Session::GetPreloads() const {
-  return preloads_;
+  auto* prefs = SessionPreferences::FromBrowserContext(browser_context());
+  DCHECK(prefs);
+  return prefs->preloads();
 }
 
 v8::Local<v8::Value> Session::Cookies(v8::Isolate* isolate) {
@@ -780,8 +782,7 @@ void Session::BuildPrototype(v8::Isolate* isolate,
       .SetMethod("getBlobData", &Session::GetBlobData)
       .SetMethod("createInterruptedDownload",
                  &Session::CreateInterruptedDownload)
-      .SetMethod("addPreload", &Session::AddPreload)
-      .SetMethod("removePreload", &Session::RemovePreload)
+      .SetMethod("setPreloads", &Session::SetPreloads)
       .SetMethod("getPreloads", &Session::GetPreloads)
       .SetProperty("cookies", &Session::Cookies)
       .SetProperty("protocol", &Session::Protocol)

atom/browser/api/atom_api_session.h

@@ -82,8 +82,7 @@ class Session: public mate::TrackableObject<Session>,
   void GetBlobData(const std::string& uuid,
                    const AtomBlobReader::CompletionCallback& callback);
   void CreateInterruptedDownload(const mate::Dictionary& options);
-  void AddPreload(const base::FilePath::StringType& preloadPath);
-  void RemovePreload(const base::FilePath::StringType& preloadPath);
+  void SetPreloads(const std::vector<base::FilePath::StringType>& preloads);
   std::vector<base::FilePath::StringType> GetPreloads() const;
   v8::Local<v8::Value> Cookies(v8::Isolate* isolate);
   v8::Local<v8::Value> Protocol(v8::Isolate* isolate);
@@ -107,7 +106,6 @@ class Session: public mate::TrackableObject<Session>,
   std::string devtools_network_emulation_client_id_;
 
   scoped_refptr<AtomBrowserContext> browser_context_;
-  std::vector<base::FilePath::StringType> preloads_;
 
   DISALLOW_COPY_AND_ASSIGN(Session);
 };

atom/browser/atom_browser_client.cc

@@ -17,6 +17,7 @@
 #include "atom/browser/atom_speech_recognition_manager_delegate.h"
 #include "atom/browser/child_web_contents_tracker.h"
 #include "atom/browser/native_window.h"
+#include "atom/browser/session_preferences.h"
 #include "atom/browser/web_contents_permission_helper.h"
 #include "atom/browser/web_contents_preferences.h"
 #include "atom/browser/window_list.h"
@@ -277,9 +278,12 @@ void AtomBrowserClient::AppendExtraCommandLineSwitches(
   }
 
   content::WebContents* web_contents = GetWebContentsFromProcessID(process_id);
-  if (web_contents)
+  if (web_contents) {
     WebContentsPreferences::AppendExtraCommandLineSwitches(
         web_contents, command_line);
+    SessionPreferences::AppendExtraCommandLineSwitches(
+        web_contents->GetBrowserContext(), command_line);
+  }
 }
 
 void AtomBrowserClient::DidCreatePpapiPlugin(

atom/browser/session_preferences.cc

@@ -0,0 +1,61 @@
+// Copyright (c) 2017 GitHub, Inc.
+// Use of this source code is governed by the MIT license that can be
+// found in the LICENSE file.
+
+#include "atom/browser/session_preferences.h"
+
+#include "atom/common/options_switches.h"
+#include "base/command_line.h"
+#include "base/memory/ptr_util.h"
+
+namespace atom {
+
+namespace {
+
+#if defined(OS_WIN)
+const base::FilePath::CharType kPathDelimiter = FILE_PATH_LITERAL(';');
+#else
+const base::FilePath::CharType kPathDelimiter = FILE_PATH_LITERAL(':');
+#endif
+
+}  // namespace
+
+// static
+int SessionPreferences::kLocatorKey = 0;
+
+SessionPreferences::SessionPreferences(content::BrowserContext* context) {
+  context->SetUserData(&kLocatorKey, base::WrapUnique(this));
+}
+
+SessionPreferences::~SessionPreferences() {
+}
+
+// static
+SessionPreferences* SessionPreferences::FromBrowserContext(
+    content::BrowserContext* context) {
+  return static_cast<SessionPreferences*>(context->GetUserData(&kLocatorKey));
+}
+
+// static
+void SessionPreferences::AppendExtraCommandLineSwitches(
+    content::BrowserContext* context, base::CommandLine* command_line) {
+  SessionPreferences* self = FromBrowserContext(context);
+  if (!self)
+    return;
+
+  base::FilePath::StringType preloads;
+  for (const auto& preload : self->preloads()) {
+    if (!base::FilePath(preload).IsAbsolute()) {
+      LOG(ERROR) << "preload script must have absolute path: " << preload;
+      continue;
+    }
+    if (preloads.empty())
+      preloads = preload;
+    else
+      preloads += kPathDelimiter + preload;
+  }
+  if (!preloads.empty())
+    command_line->AppendSwitchNative(switches::kPreloadScripts, preloads);
+}
+
+}  // namespace atom

atom/browser/session_preferences.h

@@ -0,0 +1,46 @@
+// Copyright (c) 2017 GitHub, Inc.
+// Use of this source code is governed by the MIT license that can be
+// found in the LICENSE file.
+
+#ifndef ATOM_BROWSER_SESSION_PREFERENCES_H_
+#define ATOM_BROWSER_SESSION_PREFERENCES_H_
+
+#include <vector>
+
+#include "base/files/file_path.h"
+#include "base/supports_user_data.h"
+#include "content/public/browser/browser_context.h"
+
+namespace base {
+class CommandLine;
+}
+
+namespace atom {
+
+class SessionPreferences : public base::SupportsUserData::Data {
+ public:
+  static SessionPreferences* FromBrowserContext(
+      content::BrowserContext* context);
+  static void AppendExtraCommandLineSwitches(
+      content::BrowserContext* context, base::CommandLine* command_line);
+
+  explicit SessionPreferences(content::BrowserContext* context);
+  ~SessionPreferences() override;
+
+  void set_preloads(const std::vector<base::FilePath::StringType>& preloads) {
+    preloads_ = preloads;
+  }
+  const std::vector<base::FilePath::StringType>& preloads() const {
+    return preloads_;
+  }
+
+ private:
+  // The user data key.
+  static int kLocatorKey;
+
+  std::vector<base::FilePath::StringType> preloads_;
+};
+
+}  // namespace atom
+
+#endif  // ATOM_BROWSER_SESSION_PREFERENCES_H_

atom/browser/web_contents_preferences.cc

@@ -8,9 +8,6 @@
 #include <string>
 #include <vector>
 
-#include "atom/browser/api/atom_api_session.h"
-#include "atom/browser/api/atom_api_web_contents.h"
-#include "atom/browser/api/atom_api_window.h"
 #include "atom/browser/native_window.h"
 #include "atom/browser/web_view_manager.h"
 #include "atom/common/native_mate_converters/value_converter.h"
@@ -139,19 +136,6 @@ void WebContentsPreferences::AppendExtraCommandLineSwitches(
       LOG(ERROR) << "preload url must be file:// protocol.";
   }
 
-  v8::Isolate* isolate = v8::Isolate::GetCurrent();
-  mate::Handle<atom::api::WebContents> api_web_contents =
-      atom::api::WebContents::CreateFrom(isolate, web_contents);
-  auto session = atom::api::Session::CreateFrom(
-      isolate, api_web_contents.get()->GetBrowserContext());
-  for (const auto& preloadPath : session->GetPreloads()) {
-    if (base::FilePath(preloadPath).IsAbsolute())
-      command_line->AppendSwitchNative(switches::kSessionPreloadScript,
-                                       preloadPath);
-    else
-      LOG(ERROR) << "preload script must have absolute path.";
-  }
-
   // Run Electron APIs and preload script in isolated world
   bool isolated;
   if (web_preferences.GetBoolean(options::kContextIsolation, &isolated) &&

atom/browser/web_contents_preferences.h

@@ -61,14 +61,14 @@ class WebContentsPreferences
  private:
   friend class content::WebContentsUserData<WebContentsPreferences>;
 
+  // Get preferences value as integer possibly coercing it from a string
+  bool GetInteger(const std::string& attributeName, int* intValue);
+
   static std::vector<WebContentsPreferences*> instances_;
 
   content::WebContents* web_contents_;
   base::DictionaryValue web_preferences_;
 
-  // Get preferences value as integer possibly coercing it from a string
-  bool GetInteger(const std::string& attributeName, int* intValue);
-
   DISALLOW_COPY_AND_ASSIGN(WebContentsPreferences);
 };
 

atom/common/options_switches.cc

@@ -177,18 +177,18 @@ const char kAppUserModelId[] = "app-user-model-id";
 const char kAppPath[] = "app-path";
 
 // The command line switch versions of the options.
-const char kBackgroundColor[]      = "background-color";
-const char kPreloadScript[]        = "preload";
-const char kPreloadURL[]           = "preload-url";
-const char kSessionPreloadScript[] = "session-preload";
-const char kNodeIntegration[]      = "node-integration";
-const char kContextIsolation[]     = "context-isolation";
-const char kGuestInstanceID[]      = "guest-instance-id";
-const char kOpenerID[]             = "opener-id";
-const char kScrollBounce[]         = "scroll-bounce";
-const char kHiddenPage[]           = "hidden-page";
-const char kNativeWindowOpen[]     = "native-window-open";
-const char kWebviewTag[]           = "webview-tag";
+const char kBackgroundColor[]  = "background-color";
+const char kPreloadScript[]    = "preload";
+const char kPreloadURL[]       = "preload-url";
+const char kPreloadScripts[]   = "preload-scripts";
+const char kNodeIntegration[]  = "node-integration";
+const char kContextIsolation[] = "context-isolation";
+const char kGuestInstanceID[]  = "guest-instance-id";
+const char kOpenerID[]         = "opener-id";
+const char kScrollBounce[]     = "scroll-bounce";
+const char kHiddenPage[]       = "hidden-page";
+const char kNativeWindowOpen[] = "native-window-open";
+const char kWebviewTag[]       = "webview-tag";
 
 // Command switch passed to renderer process to control nodeIntegration.
 const char kNodeIntegrationInWorker[]  = "node-integration-in-worker";

atom/common/options_switches.h

@@ -91,7 +91,7 @@ extern const char kAppPath[];
 extern const char kBackgroundColor[];
 extern const char kPreloadScript[];
 extern const char kPreloadURL[];
-extern const char kSessionPreloadScript[];
+extern const char kPreloadScripts[];
 extern const char kNodeIntegration[];
 extern const char kContextIsolation[];
 extern const char kGuestInstanceID[];

docs/api/session.md

@@ -384,22 +384,17 @@ the initial state will be `interrupted`. The download will start only when the
 
 Clears the session’s HTTP authentication cache.
 
-#### `ses.addPreload(preloadPath)`
+#### `ses.setPreloads(preloads)`
 
-* `preloadPath` String - An absolute path to the preload script
+* `preloads` String[] - An array of absolute path to preload scripts
 
-Adds a script that will be executed on ALL web contents that are associated with
+Adds scripts that will be executed on ALL web contents that are associated with
 this session just before normal `preload` scripts run.
 
-#### `ses.removePreload(preloadPath)`
-
-* `preloadPath` String - An absolute path to the preload script
-
-Removes the given script from the list of preload scripts
-
 #### `ses.getPreloads()`
 
-Returns `String[]` an array of paths to preload scripts that have been registered
+Returns `String[]` an array of paths to preload scripts that have been
+registered.
 
 ### Instance Properties
 

filenames.gypi

@@ -283,6 +283,8 @@
       'atom/browser/relauncher.h',
       'atom/browser/render_process_preferences.cc',
       'atom/browser/render_process_preferences.h',
+      'atom/browser/session_preferences.cc',
+      'atom/browser/session_preferences.h',
       'atom/browser/ui/accelerator_util.cc',
       'atom/browser/ui/accelerator_util.h',
       'atom/browser/ui/accelerator_util_mac.mm',

lib/renderer/init.js

@@ -67,7 +67,7 @@ electron.ipcRenderer.on('ELECTRON_INTERNAL_RENDERER_ASYNC_WEB_FRAME_METHOD', (ev
 let nodeIntegration = 'false'
 let webviewTag = 'false'
 let preloadScript = null
-const sessionPreloadScripts = []
+let preloadScripts = []
 let isBackgroundPage = false
 let appPath = null
 for (let arg of process.argv) {
@@ -87,11 +87,16 @@ for (let arg of process.argv) {
     appPath = arg.substr(arg.indexOf('=') + 1)
   } else if (arg.indexOf('--webview-tag=') === 0) {
     webviewTag = arg.substr(arg.indexOf('=') + 1)
-  } else if (arg.indexOf('--session-preload') === 0) {
-    sessionPreloadScripts.push(arg.substr(arg.indexOf('=') + 1))
+  } else if (arg.indexOf('--preload-scripts') === 0) {
+    preloadScripts = arg.substr(arg.indexOf('=') + 1).split(path.delimiter)
   }
 }
 
+// The webContents preload script is loaded after the session preload scripts.
+if (preloadScript) {
+  preloadScripts.push(preloadScript)
+}
+
 if (window.location.protocol === 'chrome-devtools:') {
   // Override some inspector APIs.
   require('./inspector')
@@ -174,17 +179,8 @@ if (nodeIntegration === 'true') {
   })
 }
 
-for (const sessionPreloadScript of sessionPreloadScripts) {
-  try {
-    require(sessionPreloadScript)
-  } catch (error) {
-    console.error('Unable to load session preload script: ' + sessionPreloadScript)
-    console.error(error.stack || error.message)
-  }
-}
-
-// Load the script specfied by the "preload" attribute.
-if (preloadScript) {
+// Load the preload scripts.
+for (const preloadScript of preloadScripts) {
   try {
     require(preloadScript)
   } catch (error) {

spec/api-browser-window-spec.js

@@ -1022,39 +1022,39 @@ describe('BrowserWindow module', () => {
     })
 
     describe('session preload scripts', function () {
-      it('can add and remove multiple session preload script', function () {
-        var preload = path.join(fixtures, 'module', 'set-global.js')
-        var preload2 = path.join(fixtures, 'module', 'set-global-2.js')
-        const mSession = session.defaultSession
-        assert.deepEqual(mSession.getPreloads(), [])
-        mSession.addPreload(preload)
-        assert.deepEqual(mSession.getPreloads(), [preload])
-        mSession.addPreload(preload2)
-        assert.deepEqual(mSession.getPreloads(), [preload, preload2])
-        mSession.removePreload(preload)
-        assert.deepEqual(mSession.getPreloads(), [preload2])
-        mSession.removePreload(preload2)
-        assert.deepEqual(mSession.getPreloads(), [])
+      const preloads = [
+        path.join(fixtures, 'module', 'set-global-preload-1.js'),
+        path.join(fixtures, 'module', 'set-global-preload-2.js')
+      ]
+      const defaultSession = session.defaultSession
+
+      beforeEach(() => {
+        assert.deepEqual(defaultSession.getPreloads(), [])
+        defaultSession.setPreloads(preloads)
+      })
+      afterEach(() => {
+        defaultSession.setPreloads([])
+      })
+
+      it('can set multiple session preload script', function () {
+        assert.deepEqual(defaultSession.getPreloads(), preloads)
       })
 
       it('loads the script before other scripts in window including normal preloads', function (done) {
-        var preload = path.join(fixtures, 'module', 'set-global.js')
-        var preload2 = path.join(fixtures, 'module', 'set-global-2.js')
-        const mSession = session.defaultSession
-        ipcMain.once('answer', function (event, test) {
-          mSession.removePreload(preload2)
-          assert.equal(test, 'preload2')
+        ipcMain.once('vars', function (event, preload1, preload2, preload3) {
+          assert.equal(preload1, 'preload-1')
+          assert.equal(preload2, 'preload-1-2')
+          assert.equal(preload3, 'preload-1-2-3')
           done()
         })
         w.destroy()
-        mSession.addPreload(preload2)
         w = new BrowserWindow({
           show: false,
           webPreferences: {
-            preload: preload
+            preload: path.join(fixtures, 'module', 'set-global-preload-3.js')
           }
         })
-        w.loadURL('file://' + path.join(fixtures, 'api', 'preload.html'))
+        w.loadURL('file://' + path.join(fixtures, 'api', 'preloads.html'))
       })
     })
 

spec/fixtures/api/preloads.html

@@ -0,0 +1,7 @@
+<html>
+<body>
+<script type="text/javascript" charset="utf-8">
+require('electron').ipcRenderer.send('vars', preload1, preload2, preload3);
+</script>
+</body>
+</html>

spec/fixtures/module/set-global-2.js

@@ -1 +0,0 @@
-if (!window.test) window.test = 'preload2'

spec/fixtures/module/set-global-preload-1.js

@@ -0,0 +1 @@
+window.preload1 = 'preload-1'

spec/fixtures/module/set-global-preload-2.js

@@ -0,0 +1 @@
+window.preload2 = window.preload1 + '-2'

spec/fixtures/module/set-global-preload-3.js

@@ -0,0 +1 @@
+window.preload3 = window.preload2 + '-3'