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

fix: move NativeWindow tracking to OSR WCV #15585

Merged
merged 3 commits into from Nov 30, 2018

Conversation

@adill
Copy link
Contributor

commented Nov 6, 2018

Description of Change

In #15046, when we re-enabled OSR for 4.0.x, it seems passing in a NativeWindow to the constructor of OffScreenRenderWidgetHostView became optional with the intent that SetOwnerWindow would call into SetNativeWindow -- in my experience this just isn't happening.

When SetOwnerWindow is called initially (at https://github.com/electron/electron/blob/master/atom/browser/api/atom_api_browser_window.cc#L78) content::WebContents::GetRenderWidgetHostView returns nullptr (it hasn't been created yet) and it was never called again.

I haven't dug in too deep with regards to the root causes but missing out on initialization with a NativeWindow caused my OSR window to never paint (without debugger shenanigans.) This PR restores the behavior of constructing the OffScreenRenderWidgetHostView with a valid NativeWindow.

I have no idea if this is the "right" solution, so I'm certainly open for alternate solutions, but I figured I'd open the discussion with a PR that "works for me."

NB: I tried using the NativeWindowRelay like is used elsewhere in OffScreenWebContentsView but it, of course, has no entry until SetOwnerWindow is called.

Checklist

  • PR description included and stakeholders cc'd
  • npm test passes
  • PR title follows semantic commit guidelines

Release Notes

Notes: fix: move NativeWindow tracking to OSR WCV

@adill adill added the 4-2-x label Nov 6, 2018

@adill adill self-assigned this Nov 6, 2018

@adill adill requested a review from brenca Nov 6, 2018

@adill adill requested a review from as a code owner Nov 6, 2018

@adill adill force-pushed the discordapp:fix-osr-owner-window branch from 6fe7dc6 to 31812ec Nov 6, 2018

@brenca
Copy link
Member

left a comment

We could try to make this a bit less cast-y by moving the native window tracking to OffScreenWebContentsView, that is explicitly created when the api::WebContents is created, so that should already exist when SetOwnerWindow is called.

@adill adill force-pushed the discordapp:fix-osr-owner-window branch from 31812ec to 83f9478 Nov 6, 2018

@adill adill requested review from as code owners Nov 6, 2018

@adill adill changed the base branch from 4-0-x to master Nov 6, 2018

@adill

This comment has been minimized.

Copy link
Contributor Author

commented Nov 6, 2018

@brenca Thanks! I updated the branch with your patch.

@adill adill changed the title fix: pass NativeWindow to OSR WHV to ensure it initializes correctly fix: move NativeWindow tracking to OSR WCV Nov 6, 2018

@brenca

brenca approved these changes Nov 7, 2018

Copy link
Member

left a comment

lgtm, but would like a second opinion, @codebytere maybe?

@groundwater groundwater requested review from ckerr and codebytere Nov 13, 2018

@groundwater
Copy link
Member

left a comment

Approve to get groups checked, but still waiting on @ckerr @codebytere

@codebytere

This comment has been minimized.

Copy link
Member

commented Nov 17, 2018

@zcbenz this needs your approval for the two missing codeowner teams when you have a moment :)

@zcbenz

zcbenz approved these changes Nov 17, 2018

ckerr added some commits Nov 29, 2018

@codebytere codebytere merged commit 8cca1c9 into electron:master Nov 30, 2018

23 checks passed

Absolute Zero
Artifact Comparison No Changes
Details
Semantic Pull Request ready to be squashed
Details
WIP ready for review
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-x64-testing-pr AppVeyor build succeeded
Details
ci/circleci: linux-arm-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-arm-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-arm64-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-arm64-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-checkout Your tests passed on CircleCI!
Details
ci/circleci: linux-ia32-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-ia32-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-ia32-testing-tests Your tests passed on CircleCI!
Details
ci/circleci: linux-x64-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-x64-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-x64-testing-tests Your tests passed on CircleCI!
Details
ci/circleci: mas-testing Your tests passed on CircleCI!
Details
ci/circleci: mas-testing-tests Your tests passed on CircleCI!
Details
ci/circleci: osx-testing Your tests passed on CircleCI!
Details
ci/circleci: osx-testing-tests Your tests passed on CircleCI!
Details
electron-lint Build #20181129.18 succeeded
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

Copy link

commented Nov 30, 2018

Release Notes Persisted

fix: move NativeWindow tracking to OSR WCV

@adill adill deleted the discordapp:fix-osr-owner-window branch Dec 5, 2018

@MarshallOfSound MarshallOfSound added target/4-0-x and removed 4-2-x labels Dec 6, 2018

@codebytere

This comment has been minimized.

Copy link
Member

commented Dec 6, 2018

/trop run backport-to 4-0-x

@trop

This comment has been minimized.

Copy link
Contributor

commented Dec 6, 2018

The backport process for this PR has been manually initiated,
sending your 1's and 0's to "4-0-x" here we go! :D

@trop

This comment has been minimized.

Copy link
Contributor

commented Dec 6, 2018

I was unable to backport this PR to "4-0-x" cleanly;
you will need to perform this backport manually.

codebytere added a commit that referenced this pull request Dec 6, 2018

fix: move NativeWindow tracking to OSR WCV (#15585)
* fix: move NativeWindow tracking to OSR WCV

* fix oops

codebytere added a commit that referenced this pull request Dec 10, 2018

fix: move NativeWindow tracking to OSR WCV (#15585) (#15970)
* fix: move NativeWindow tracking to OSR WCV

* fix oops
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.