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

Implement invalidate for non-offscreen mode #8628

Merged
merged 8 commits into from Feb 14, 2017

Conversation

Projects
None yet
4 participants
@Spacetech
Contributor

Spacetech commented Feb 8, 2017

This PR implements the webContents.invalidate method for non-offscreen mode. It will schedule a paint to the native window.

An example use case for this method: A few games on Steam use Electron and Greenworks. Steam games have an in-game overlay that shows up over the game, which users can toggle via a keybind. This overlay has many issues in electron apps.

The main issue is that since the overlay is hooked into the renderer, the overlay is frozen / corrupted when the renderer isn't painting or if it's only painting small dirty regions. We used to implement some shady methods to force repaints, such as having multiple 1x1 pixel canvases refreshing, or translateZ(0)ing some elements.. but these hacks stopped working properly in newer Chromium versions. See Greenworks#50 for detailed discussion.
Calling this invalidate method often (60fps / every animation frame) completely solves this issue.

@kevinsawicki

This comment has been minimized.

Show comment
Hide comment
@kevinsawicki

kevinsawicki Feb 9, 2017

Contributor

@Spacetech what would be the best way to test this out to see it resolving the Greenworks issue you mentioned? I've never used Greenworks before and don't have any Steam games using Electron currently, thanks.

Contributor

kevinsawicki commented Feb 9, 2017

@Spacetech what would be the best way to test this out to see it resolving the Greenworks issue you mentioned? I've never used Greenworks before and don't have any Steam games using Electron currently, thanks.

@Spacetech

This comment has been minimized.

Show comment
Hide comment
@Spacetech

Spacetech Feb 10, 2017

Contributor

Testing it is simple since Steam supports non-steam games. I setup a repo with the instructions; https://github.com/Spacetech/electronsteamoverlaytest

Greenworks isn't related to the overlay issue itself. I brought it up since most Node.js based Steam games use it and because that Electron + Steam Overlay issue on their tracker was quite active.

Let me know if you have any questions. Thanks!

Contributor

Spacetech commented Feb 10, 2017

Testing it is simple since Steam supports non-steam games. I setup a repo with the instructions; https://github.com/Spacetech/electronsteamoverlaytest

Greenworks isn't related to the overlay issue itself. I brought it up since most Node.js based Steam games use it and because that Electron + Steam Overlay issue on their tracker was quite active.

Let me know if you have any questions. Thanks!

@kevinsawicki

This comment has been minimized.

Show comment
Hide comment
@kevinsawicki

kevinsawicki Feb 13, 2017

Contributor

Just a heads up, looks like this doesn't compile on macOS, getNativeWindow returns an NSWindow on that platform:

../../atom/browser/api/atom_api_web_contents.cc:1502:45: error: member access into incomplete type 'NSWindow'
      const gfx::Rect& bounds = nativeWindow->bounds();
                                            ^
/Users/travis/build/electron/electron/vendor/brightray/vendor/download/libchromiumcontent/src/ui/gfx/native_widget_types.h:88:7: note: forward declaration of 'NSWindow'
class NSWindow;
      ^
../../atom/browser/api/atom_api_web_contents.cc:1503:19: error: member access into incomplete type 'NSWindow'
      nativeWindow->SchedulePaintInRect(
                  ^
/Users/travis/build/electron/electron/vendor/brightray/vendor/download/libchromiumcontent/src/ui/gfx/native_widget_types.h:88:7: note: forward declaration of 'NSWindow'
class NSWindow;
Contributor

kevinsawicki commented Feb 13, 2017

Just a heads up, looks like this doesn't compile on macOS, getNativeWindow returns an NSWindow on that platform:

../../atom/browser/api/atom_api_web_contents.cc:1502:45: error: member access into incomplete type 'NSWindow'
      const gfx::Rect& bounds = nativeWindow->bounds();
                                            ^
/Users/travis/build/electron/electron/vendor/brightray/vendor/download/libchromiumcontent/src/ui/gfx/native_widget_types.h:88:7: note: forward declaration of 'NSWindow'
class NSWindow;
      ^
../../atom/browser/api/atom_api_web_contents.cc:1503:19: error: member access into incomplete type 'NSWindow'
      nativeWindow->SchedulePaintInRect(
                  ^
/Users/travis/build/electron/electron/vendor/brightray/vendor/download/libchromiumcontent/src/ui/gfx/native_widget_types.h:88:7: note: forward declaration of 'NSWindow'
class NSWindow;
@Spacetech

This comment has been minimized.

Show comment
Hide comment
@Spacetech

Spacetech Feb 14, 2017

Contributor

Thanks for letting me know. I implemented a mac version that should work, however I can't test it since I don't have a mac. Also the Steam Overlay with Electron never worked on mac so I'm not sure how I would test it.

I added an Invalidate method to the NativeWindow class.
Looking at the apis, it seems like invalidate should be added to BrowserWindow instead of WebContents. But both having an invalidate method might be weird. Thoughts?

Contributor

Spacetech commented Feb 14, 2017

Thanks for letting me know. I implemented a mac version that should work, however I can't test it since I don't have a mac. Also the Steam Overlay with Electron never worked on mac so I'm not sure how I would test it.

I added an Invalidate method to the NativeWindow class.
Looking at the apis, it seems like invalidate should be added to BrowserWindow instead of WebContents. But both having an invalidate method might be weird. Thoughts?

@kevinsawicki

This comment has been minimized.

Show comment
Hide comment
@kevinsawicki

kevinsawicki Feb 14, 2017

Contributor

But both having an invalidate method might be weird. Thoughts?

I think it is okay for now to have it only publicly available on the webContents even though it just delegates to the owner window in non-offscreen mode. Since this API is relatively new it feels like something that might still be tweaked as people start using it and we can revisit the best home for it in the future as it evolves.

Contributor

kevinsawicki commented Feb 14, 2017

But both having an invalidate method might be weird. Thoughts?

I think it is okay for now to have it only publicly available on the webContents even though it just delegates to the owner window in non-offscreen mode. Since this API is relatively new it feels like something that might still be tweaked as people start using it and we can revisit the best home for it in the future as it evolves.

@kevinsawicki

This comment has been minimized.

Show comment
Hide comment
@kevinsawicki

kevinsawicki Feb 14, 2017

Contributor

Also the Steam Overlay with Electron never worked on mac so I'm not sure how I would test it.

I tried and couldn't get it working, I think it is okay to ship this in its current form though.

Contributor

kevinsawicki commented Feb 14, 2017

Also the Steam Overlay with Electron never worked on mac so I'm not sure how I would test it.

I tried and couldn't get it working, I think it is okay to ship this in its current form though.

@kevinsawicki

This comment has been minimized.

Show comment
Hide comment
@kevinsawicki

kevinsawicki Feb 14, 2017

Contributor

Testing it is simple since Steam supports non-steam games. I setup a repo with the instructions; https://github.com/Spacetech/electronsteamoverlaytest

Thanks for providing this, was able to build this branch on Windows 10 and verify the Steam overlay shows up and can be interacted with 👍

Contributor

kevinsawicki commented Feb 14, 2017

Testing it is simple since Steam supports non-steam games. I setup a repo with the instructions; https://github.com/Spacetech/electronsteamoverlaytest

Thanks for providing this, was able to build this branch on Windows 10 and verify the Steam overlay shows up and can be interacted with 👍

@kevinsawicki kevinsawicki merged commit 9be42db into electron:master Feb 14, 2017

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@kevinsawicki

This comment has been minimized.

Show comment
Hide comment
@kevinsawicki

kevinsawicki Feb 14, 2017

Contributor

Great job on this @Spacetech, thanks for adding support for it 🚢 :shipit: 👍

Contributor

kevinsawicki commented Feb 14, 2017

Great job on this @Spacetech, thanks for adding support for it 🚢 :shipit: 👍

@Spacetech

This comment has been minimized.

Show comment
Hide comment
@Spacetech

Spacetech Feb 14, 2017

Contributor

Thanks for reviewing and merging it

Contributor

Spacetech commented Feb 14, 2017

Thanks for reviewing and merging it

@hokein

This comment has been minimized.

Show comment
Hide comment
@hokein

hokein Feb 14, 2017

Contributor

@Spacetech Awesome! Just see it now (I should have noticed it earlier). I will check whether this solution works on OS X and Linux.

Contributor

hokein commented Feb 14, 2017

@Spacetech Awesome! Just see it now (I should have noticed it earlier). I will check whether this solution works on OS X and Linux.

@vaughnroyko

This comment has been minimized.

Show comment
Hide comment
@vaughnroyko

vaughnroyko Feb 14, 2017

@hokein Unfortunately the overlay will not work on OS X and Linux, even with this change, due to some changes that happened internally in Chromium at some point. I haven't found the version in which it broke in Electron; however, we found the version earlier in NW.js: greenheartgames/greenworks#28 - Would love if we could track down the cause though!

vaughnroyko commented Feb 14, 2017

@hokein Unfortunately the overlay will not work on OS X and Linux, even with this change, due to some changes that happened internally in Chromium at some point. I haven't found the version in which it broke in Electron; however, we found the version earlier in NW.js: greenheartgames/greenworks#28 - Would love if we could track down the cause though!

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