Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: simplify content script injection #18532

Merged
merged 3 commits into from Jun 4, 2019

Conversation

Projects
None yet
4 participants
@nornagon
Copy link
Contributor

commented May 30, 2019

Description of Change

This replaces the substantial RenderProcessPreferences machinery with a much simpler synchronous IPC to fetch the content scripts that need injecting. Ultimately this should be replaced by the real extensions API (ref #17440), but this works for now and lets us remove a bunch of code.

Checklist

Release Notes

Notes: no-notes

@nornagon nornagon requested a review from deepak1556 May 30, 2019

V(atom_browser_power_monitor) \
V(atom_browser_power_save_blocker) \
V(atom_browser_protocol) \
V(atom_browser_render_process_preferences) \

This comment has been minimized.

Copy link
@nornagon

nornagon May 30, 2019

Author Contributor

this is the line i deleted (cross-check with ?w=1)

@MarshallOfSound

This comment has been minimized.

Copy link
Member

commented May 31, 2019

Do we know the reasoning for the original design, inspired by Chromium, performance, etc?

@nornagon

This comment has been minimized.

Copy link
Contributor Author

commented May 31, 2019

cc @zcbenz who wrote this initially, I believe

@nornagon

This comment has been minimized.

Copy link
Contributor Author

commented May 31, 2019

I think this also fixes a potential race condition maybe? It's not clear to me that the AtomMsg_UpdatePreferences IPC is guaranteed to arrive before the content script injector runs.

@deepak1556
Copy link
Member

left a comment

LGTM 👍

Requesting @zcbenz review, since he wrote this implementation.

content::WebContents* web_contents =
AtomBrowserClient::Get()->GetWebContentsFromProcessID(host->GetID());
if (web_contents)
WebContents::FromOrCreate(v8::Isolate::GetCurrent(), web_contents);

This comment has been minimized.

Copy link
@deepak1556

deepak1556 May 31, 2019

Member

IIUC the previous version in IsWebContents was only there to make sure we update preferences for BrowserWindow or WebView and not for a devtools Remote type. Since the ipc was sent when render process being created without knowing what its contents were. But in this PR, the ipc is made by the renderer process so it chooses the right target WebContents, so i don't think this wrapper creation is necessary.

This comment has been minimized.

Copy link
@nornagon

nornagon May 31, 2019

Author Contributor

If this line is deleted, devtools doesn't open.

This comment has been minimized.

Copy link
@deepak1556

deepak1556 Jun 3, 2019

Member

@zcbenz whats the intent behind wrapping api::WebContents at render process start for devtools ?

This comment has been minimized.

Copy link
@zcbenz

zcbenz Jun 4, 2019

Member

Hmm I am not sure why this is related to devtools, I would need to do some debugging to know.

This comment has been minimized.

Copy link
@nornagon

nornagon Jun 4, 2019

Author Contributor

Given that this preserves the existing behaviour (i.e. calling WebContents::FromOrCreate on every render process at creation time), I'm going to go ahead and merge this, but it does seem odd.

@electron-cation electron-cation bot removed the new-pr 🌱 label May 31, 2019

@zcbenz

zcbenz approved these changes Jun 1, 2019

Copy link
Member

left a comment

The original motivation was to avoid sending synchronous IPC message on page load, but since we are going to use Chromium's extensions API implementation, this does not really matter any more.

@zcbenz

This comment has been minimized.

Copy link
Member

commented Jun 2, 2019

I think this also fixes a potential race condition maybe? It's not clear to me that the AtomMsg_UpdatePreferences IPC is guaranteed to arrive before the content script injector runs.

Sending IPC messages immediately after renderer process gets created should be able to guarantee the message get received before page loading, since page loading is started by sending IPC messages too. This pattern is copied from Chromium.

@MarshallOfSound
Copy link
Member

left a comment

Requesting changes so we can confirm the numbers outlined in #18532 (comment)

@nornagon nornagon force-pushed the simplify-content-script-injection branch from d45f1ed to a14454b Jun 3, 2019

@nornagon

This comment has been minimized.

Copy link
Contributor Author

commented Jun 3, 2019

1.6ms is still longer than I'd like, but I think it's within acceptable range considering the reduction in complexity and that we'll eventually be switching to "real" extensions.

@nornagon nornagon requested a review from MarshallOfSound Jun 3, 2019

@nornagon nornagon force-pushed the simplify-content-script-injection branch from a14454b to beb8f39 Jun 4, 2019

@nornagon

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2019

linux failures are flake (see #18630), merging.

@nornagon nornagon merged commit ed5fb4a into master Jun 4, 2019

12 of 13 checks passed

build-linux Workflow: build-linux
Details
Artifact Comparison No Changes
Details
Semantic Pull Request ready to be squashed
Details
WIP Ready for review
Details
appveyor: win-ia32-testing AppVeyor build succeeded
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-x64-testing AppVeyor build succeeded
Details
appveyor: win-x64-testing-pr AppVeyor build succeeded
Details
build-mac Workflow: build-mac
Details
electron-arm-testing Build #20190604.57 succeeded
Details
electron-arm64-testing Build #20190604.57 succeeded
Details
lint Workflow: lint
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

Copy link

commented Jun 4, 2019

No Release Notes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.