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: send NSView* as the response to getNativeWindowHandle() instead of a null handle #15521

Merged
merged 1 commit into from Nov 8, 2018

Conversation

MarshallOfSound
Copy link
Member

@MarshallOfSound MarshallOfSound commented Nov 1, 2018

Fixes #15489

Notes: Fixed issue where getNativeWindowHandle() would return an empty buffer on macOS

Originally caused by 4068a62 which was caused by https://chromium-review.googlesource.com/c/chromium/src/+/792295

This re-implements the API on macOS without using gfx::AcceleratedWidget

cc @deepak1556

@deepak1556
Copy link
Member

deepak1556 commented Nov 6, 2018

Can you add a TODO to restore the old implementation once https://chromium-review.googlesource.com/c/chromium/src/+/1253094/ lands

gfx::AcceleratedWidget NativeWindowMac::GetAcceleratedWidget() const {
  auto* bridged_native_widget = 
      views::BridgedNativeWidgetHostImpl::GetFromNativeWindow(window_);
  return bridged_native_widget->GetRootViewNSViewId();
}

and respective view can be extracted with ui::NSViewIds::GetNSView

Copy link
Member

@deepak1556 deepak1556 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Current implementation looks good, naming could beGetNativeWindowHandlePointer => GetHandle ?

@MarshallOfSound
Copy link
Member Author

@deepak1556 Initially had it as GetHandle but that conflicted with the concept of a mate::Handle. Added the TODO

@MarshallOfSound MarshallOfSound force-pushed the fix-get-native-window-handle-macos branch from fe2a6b0 to 8bdea21 Compare November 7, 2018 03:21
@deepak1556
Copy link
Member

Initially had it as GetHandle but that conflicted with the concept of a mate::Handle

If you changed TopLevelWindow::GetNativeWindowHandle , I can see the conflict but I don't see how it conflicts when changing NativeWindow::GetNativeWindowHandlePointer to NativeWindow::GetHandle.

Also can you fix the lint failures ? Thanks!

@MarshallOfSound
Copy link
Member Author

@deepak1556 Linting is fixed, let's get this one through 😄

Copy link
Member

@deepak1556 deepak1556 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation will be changed again in C71, so I am good with current naming convention.

@codebytere codebytere merged commit 9aed2a4 into master Nov 8, 2018
@release-clerk
Copy link

release-clerk bot commented Nov 8, 2018

Release Notes Persisted

Fixed issue where getNativeWindowHandle() would return an empty buffer on macOS

@codebytere codebytere deleted the fix-get-native-window-handle-macos branch November 8, 2018 17:03
@trop
Copy link
Contributor

trop bot commented Nov 8, 2018

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

@trop
Copy link
Contributor

trop bot commented Nov 8, 2018

I have automatically backported this PR to "4-0-x", please check out #15644

@tommoor
Copy link
Contributor

tommoor commented Nov 20, 2018

Any chance of this being backported to 3.X? 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
3.1.x
Fixed in 3.1.0-beta.1
Development

Successfully merging this pull request may close these issues.

None yet

4 participants