Add ability to set per-session preload scripts #10430

MarshallOfSound · 2017-09-02T16:18:59Z

This adds three new instance methods to session objects that allow you to set preload scripts, these scripts will be executed before normal preload scripts and you can have as many as you want.

API:

#### `ses.addPreload(preloadPath)`

* `preloadPath` String - An absolute path to the preload script

Adds a script 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

poiru · 2017-09-04T17:06:45Z

atom/browser/api/atom_api_window.h

@@ -54,6 +54,10 @@ class Window : public mate::TrackableObject<Window>,

  int32_t ID() const;

+  static void AddGlobalPreload(const base::FilePath::StringType preloadPath);
+  static void RemoveGlobalPreload(const base::FilePath::StringType preloadPath);


Could this be const base::FilePath::StringType& (and std::vector<base::FilePath::StringType>& below) to avoid a copies?

poiru

I'm curious what's the use case for this? Also, should we generalize this so that it can be used for other web contents as well (e.g. BrowserView)?

MarshallOfSound · 2017-09-05T07:52:47Z

@poiru Two main use cases for me

It allows electron modules to do stuff in renderer processes when initialized from the main process, modules such as electron-compile currently have to do fancy hax to inject initialization code into renderer processes.
It lets people have multiple preload scripts
It allows for generic "all window" setup logic (for instance custom window controls)

MarshallOfSound · 2017-09-05T08:00:50Z

Also, should we generalize this so that it can be used for other web contents as well (e.g. BrowserView)?

I'm pretty sure this already works for BrowserView's as they still run the init.js script. The API is on BrowserWindow as it is the hierarchical parent of all webContents, even browser views are attached to browser windows.

poiru · 2017-09-05T15:17:55Z

I'm pretty sure this already works for BrowserView's as they still run the init.js script. The API is on BrowserWindow as it is the hierarchical parent of all webContents, even browser views are attached to browser windows.

Ah, I see! Since BrowserViews don't necessarily have to be attached to a window, perhaps a static methods on webContents might be better?

MarshallOfSound · 2017-09-05T15:25:31Z

perhaps a static methods on webContents might be better?

I initially thought about putting them there but then decided against it because existing preloads go onto the BrowserWindow constructor. Happy to move it somewhere else if it makes more sense

deepak1556 · 2017-09-05T15:31:20Z

Happy to move it somewhere else if it makes more sense

I would suggest adding it to the session module #2605

MarshallOfSound · 2017-09-05T15:32:54Z

@deepak1556 At the moment this isn't session scoped though, it's for all sessions. That issue refers to preloads for iframes as well, not sure this PR quite hits that issue perfectly.

deepak1556 · 2017-09-05T15:41:39Z

Having it scoped to session gives better control, doesn't have to worry about implementation for webContents, BrowserView, webView, BrowserWindow.

it's for all sessions

If the scripts are added to the global session, then it would address the use case you mentioned too.

Any reason why this shouldn't be session specific ?

zcbenz · 2017-09-11T02:58:27Z

I think it is better to be a session-wide API instead of a global API.

For single-session apps setting the preload script for the global session is as easy as setting the global preload script.

For multi-session apps, like browsers that use different sessions for UI and remote pages, they almost never use the same preload script for different sessions. So an API setting global preload script would be useless for those apps, while an API settings session-wide preload script can meet most needs.

MarshallOfSound · 2017-09-16T15:09:37Z

@zcbenz @deepak1556 Moved the logic to be session based 👍

poiru · 2017-09-18T11:03:39Z

atom/browser/api/atom_api_session.h

@@ -81,6 +82,9 @@ 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);
+  std::vector<base::FilePath::StringType> GetPreloads();


Could you use const std::vector<base::FilePath::StringType>& to avoid copying.

poiru · 2017-09-18T11:04:29Z

atom/browser/api/atom_api_session.h

@@ -103,6 +107,7 @@ class Session: public mate::TrackableObject<Session>,
  std::string devtools_network_emulation_client_id_;

  scoped_refptr<AtomBrowserContext> browser_context_;
+  std::vector<base::FilePath::StringType> g_preloads;


preload_paths_ is probably a better name.

poiru · 2017-09-18T11:04:38Z

atom/browser/api/atom_api_session.h

@@ -81,6 +82,9 @@ 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);


s/preloadPath/preload_path

poiru · 2017-09-18T11:05:06Z

atom/browser/web_contents_preferences.cc

+    atom::api::WebContents::CreateFrom(isolate, web_contents);
+  auto session = atom::api::Session::CreateFrom(
+    isolate, api_web_contents.get()->GetBrowserContext());
+  for (auto preloadPath : session->GetPreloads()) {


const auto& to avoid copy. Also preload_path.

poiru · 2017-09-18T11:05:50Z

atom/browser/web_contents_preferences.cc

+  auto session = atom::api::Session::CreateFrom(
+    isolate, api_web_contents.get()->GetBrowserContext());
+  for (auto preloadPath : session->GetPreloads()) {
+    if (base::FilePath(preloadPath).IsAbsolute())


Would it be better to do this check in AddPreload()?

For consistency with existing preload logic I think it's better to leave this here

zcbenz · 2017-12-05T02:36:20Z

I'm rebasing this branch on master.

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

deepak1556

LGTM 👍

deepak1556 · 2017-12-05T07:41:47Z

atom/browser/api/atom_api_web_contents.h

@@ -367,8 +368,6 @@ class WebContents : public mate::TrackableObject<WebContents>,
                         const std::vector<base::string16>& labels);

 private:
-  AtomBrowserContext* GetBrowserContext() const;


Do we want this method to be public ? Its not used in this PR.

deepak1556 · 2017-12-05T07:44:39Z

atom/browser/session_preferences.cc

+}  // namespace
+
+// static
+int SessionPreferences::kLocatorKey = 0;


Would it be better to have the key as SessionPreferencesUserData just so it doesn't collide with anything from content layer in the future ?

The static member data has the class name in its symbol name, so we won't get symbol conflicts. The kLocatorKey is a pattern also used in other Chromium classes.

carsonwright · 2017-12-12T09:26:48Z

What would the js api look like for this?

iffy · 2017-12-12T22:07:05Z

How will I know when this feature is released? (I've scoured the website looking for an RSS feed or a mailing list. Closest thing I can find is Twitter)

lee-dohm · 2017-12-13T20:52:02Z

@iffy You can subscribe to an Atom feed of the releases or tags for any repository. Whether or not a particular PR is "released" is rather more complicated but hopefully that should get you started.

MarshallOfSound requested a review from a team September 2, 2017 16:23

poiru reviewed Sep 4, 2017

View reviewed changes

MarshallOfSound changed the title ~~Add ability to set Global Preload scripts for all BrowserWindows~~ Add ability to set per-session preload scripts Sep 16, 2017

MarshallOfSound force-pushed the global-preloads branch from 7a6c22a to 7ab1e42 Compare September 16, 2017 15:09

MarshallOfSound force-pushed the global-preloads branch from 01f1a70 to b719fe7 Compare September 16, 2017 15:20

poiru reviewed Sep 18, 2017

View reviewed changes

qazbnm456 mentioned this pull request Sep 29, 2017

Session wide preload script LulumiProject/lulumi-browser#24

Open

zcbenz self-assigned this Dec 5, 2017

MarshallOfSound added 9 commits December 5, 2017 11:35

Add ability to set global preload scripts

0ddd078

Use const references

448ccc2

Move global preload implementation to be session based

3b80ee0

s/global/session

d270731

s/global/session in constants

d9359d8

Appease the almighty linter

d1aded2

Don't know why we suddenly need this, but hey :/

e535ba4

Standard JS

009459b

Remove .only

eaa4b71

zcbenz force-pushed the global-preloads branch from 3ffbc42 to eaa4b71 Compare December 5, 2017 02:36

MarshallOfSound requested a review from a team December 5, 2017 02:36

zcbenz added 2 commits December 5, 2017 11:50

Coding style changes

776e8af

Add a SessionPreferences to manage session related data

cb3a9c6

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

zcbenz closed this Dec 5, 2017

zcbenz reopened this Dec 5, 2017

deepak1556 approved these changes Dec 5, 2017

View reviewed changes

Revert some unnecessary changes

24b3ee3

zcbenz approved these changes Dec 5, 2017

View reviewed changes

zcbenz merged commit 95cb601 into master Dec 5, 2017

zcbenz deleted the global-preloads branch December 5, 2017 10:15

iffy mentioned this pull request Jan 5, 2018

Support macros for banks that use iframes buckets/application#23

Closed

codebytere mentioned this pull request Jan 25, 2018

Session wide preload script #2605

Closed

pfrazee mentioned this pull request Apr 2, 2018

Support Node / Preload scripts in iframes or workers #12505

Closed

iffy mentioned this pull request Apr 25, 2018

Support macros for banks that use iframes buckets/application#132

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability to set per-session preload scripts #10430

Add ability to set per-session preload scripts #10430

MarshallOfSound commented Sep 2, 2017 •

edited

Loading

poiru Sep 4, 2017 •

edited

Loading

MarshallOfSound Sep 5, 2017

poiru left a comment

MarshallOfSound commented Sep 5, 2017

MarshallOfSound commented Sep 5, 2017

poiru commented Sep 5, 2017

MarshallOfSound commented Sep 5, 2017

deepak1556 commented Sep 5, 2017

MarshallOfSound commented Sep 5, 2017

deepak1556 commented Sep 5, 2017

zcbenz commented Sep 11, 2017

MarshallOfSound commented Sep 16, 2017

poiru Sep 18, 2017

poiru Sep 18, 2017 •

edited

Loading

poiru Sep 18, 2017

poiru Sep 18, 2017

poiru Sep 18, 2017

MarshallOfSound Oct 27, 2017

zcbenz commented Dec 5, 2017

deepak1556 left a comment

deepak1556 Dec 5, 2017

deepak1556 Dec 5, 2017

zcbenz Dec 5, 2017

deepak1556 Dec 5, 2017

carsonwright commented Dec 12, 2017

iffy commented Dec 12, 2017

lee-dohm commented Dec 13, 2017

Add ability to set per-session preload scripts #10430

Add ability to set per-session preload scripts #10430

Conversation

MarshallOfSound commented Sep 2, 2017 • edited Loading

poiru Sep 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poiru left a comment

Choose a reason for hiding this comment

MarshallOfSound commented Sep 5, 2017

MarshallOfSound commented Sep 5, 2017

poiru commented Sep 5, 2017

MarshallOfSound commented Sep 5, 2017

deepak1556 commented Sep 5, 2017

MarshallOfSound commented Sep 5, 2017

deepak1556 commented Sep 5, 2017

zcbenz commented Sep 11, 2017

MarshallOfSound commented Sep 16, 2017

Choose a reason for hiding this comment

poiru Sep 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zcbenz commented Dec 5, 2017

deepak1556 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carsonwright commented Dec 12, 2017

iffy commented Dec 12, 2017

lee-dohm commented Dec 13, 2017

MarshallOfSound commented Sep 2, 2017 •

edited

Loading

poiru Sep 4, 2017 •

edited

Loading

poiru Sep 18, 2017 •

edited

Loading