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

Support document visibility and events in webviews #5684

Merged
merged 14 commits into from May 26, 2016

Conversation

Projects
None yet
2 participants
@kevinsawicki
Contributor

kevinsawicki commented May 24, 2016

  • Add support for visibliitychange document events in <webview> tags
  • Inherit the initial value of document.hidden and document.visibilityState from parent BrowserWindow
  • Add tests of initial state and a change event

Closes #4879

this.onVisibilityChanged = (event, visibilityState) => {
this.webviewNode.send('ELECTRON_RENDERER_WINDOW_VISIBILITY_CHANGE', visibilityState)
}
ipcRenderer.on('ELECTRON_RENDERER_WINDOW_VISIBILITY_CHANGE', this.onVisibilityChanged)

This comment has been minimized.

@zcbenz

zcbenz May 25, 2016

Contributor

We can probably listen to the DOM visibilitychange event, to reduce code duplication.

This comment has been minimized.

@kevinsawicki

kevinsawicki May 25, 2016

Contributor

Hmm, but line 47 would still need to use that IPC event name right when calling this.webviewNode.send? I kind of like using the IPC event both places since it makes it clear we are just forwarding the event into the webview.

This comment has been minimized.

@zcbenz

zcbenz May 26, 2016

Contributor

👍

@@ -160,7 +160,14 @@ void WebContentsPreferences::AppendExtraCommandLineSwitches(
if (window) {
bool visible = window->IsVisible() && !window->IsMinimized();
if (!visible) // Default state is visible.
command_line->AppendSwitch("hidden-page");
command_line->AppendSwitch(switches::kHiddenPage);
} else {

This comment has been minimized.

@zcbenz

zcbenz May 25, 2016

Contributor

A WebContents without window doesn't not necessarily mean it is a webview, it can be an anonymous WebContents created by electron.webContents.create(), this API is not public yet but we might make it public in future.

The safe way is to check whether guest_instance_id is set.

} else {
// Inherit initial visibilty state from parent window in webviews
bool hidden_page;
if (web_preferences.GetBoolean(options::kHiddenPage, &hidden_page) &&

This comment has been minimized.

@zcbenz

zcbenz May 25, 2016

Contributor

I tend to implement this with native code instead of adding a new param to guest contents, every time we do so it would make code harder to maintain.

The relationship between host and webview WebContents is maintained by WebViewManager, we can add a new API there to receive the host WebContents of a webview, and then reuse the logic for normal WebContents.

if (guest_instance_id && !window) {
auto manager = WebViewManager::GetWebViewManager(web_contents);
if (manager) {
content::WebContents* embedder = manager->GetEmbedder(guest_instance_id);

This comment has been minimized.

@kevinsawicki

kevinsawicki May 25, 2016

Contributor

@zcbenz Thanks for pointing out web_view_manager. I added a new API to get the embedder web contents for a guest instance id which allows window lookup.

Is this in line with what you were suggesting?

I've removed the passing of hiddenPage in the params via this approach, thanks for mentioning that, I think this approach is cleaner.

This comment has been minimized.

@zcbenz

zcbenz May 26, 2016

Contributor

👍

kevinsawicki added some commits May 25, 2016

@@ -157,6 +158,17 @@ void WebContentsPreferences::AppendExtraCommandLineSwitches(
// The initial visibility state.
NativeWindow* window = NativeWindow::FromWebContents(web_contents);
// Use embedder window for webviews
if (guest_instance_id && !window) {

This comment has been minimized.

@zcbenz

zcbenz May 26, 2016

Contributor

The guest_instance_id needs to be initialized above, otherwise it could be filled with random data.

This comment has been minimized.

@kevinsawicki

kevinsawicki May 26, 2016

Contributor

Whoops, thanks for mentioning that, pushed a commit that initializes it to 0.

@zcbenz zcbenz merged commit 4ea7602 into master May 26, 2016

0 of 4 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
electron-linux-arm Build #3306815 queued
Details
electron-linux-ia32 Build #3306816 queued
Details
electron-linux-x64 Build #3306817 queued
Details

@zcbenz zcbenz deleted the webview-visibilitychange branch May 26, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment