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

Explicitly hide tooltip when the window is deactivated (master) #11644

Merged
merged 5 commits into from Jan 19, 2018

Conversation

Projects
None yet
3 participants
@brenca
Copy link
Member

brenca commented Jan 15, 2018

This is here to fix #9943 on master

@brenca brenca requested a review from electron/reviewers as a code owner Jan 15, 2018

@alexeykuzmin

This comment has been minimized.

Copy link
Contributor

alexeykuzmin commented Jan 18, 2018

@brenca Can you please rebase your changes onto the latest master to fix the electron-linux-ia32 failure?

@brenca brenca force-pushed the brenca:tooltip-fix-master branch from 2a34390 to 8d8b8c5 Jan 18, 2018

@brenca

This comment has been minimized.

Copy link
Member Author

brenca commented Jan 18, 2018

@alexeykuzmin done!

@ckerr
Copy link
Member

ckerr left a comment

Looks good in principle, have a couple of questions inline that might or might not be issues


namespace atom {

AtomDesktopNativeWidgetAura::AtomDesktopNativeWidgetAura(
views::internal::NativeWidgetDelegate* delegate)
: views::DesktopNativeWidgetAura(delegate) {
// This is to enable the override of OnWindowActivated
wm::SetActivationChangeObserver(GetNativeWindow(), this);

This comment has been minimized.

@ckerr

ckerr Jan 18, 2018

Member

From the libcc API, it looks like we can only have one observer:

WM_PUBLIC_EXPORT void SetActivationChangeObserver(
    aura::Window* window,
    ActivationChangeObserver* observer);
WM_PUBLIC_EXPORT ActivationChangeObserver* GetActivationChangeObserver(
    aura::Window* window);

Are we certain that there's not some another observer being overwriting here?

This comment has been minimized.

@brenca

brenca Jan 18, 2018

Author Member

Our AtomDesktopNativeWidgetAura is a subclass of the original views::DesktopNativeWidgetAura, which in the contructor calls SetActivationChangeObserver with the content_window_ and this. In our case, we call it with the same thing, since GetNativeWindow returns content_window_, and this still behaves the same as before (except for the fact that the subclass method gets called now in case of OnWindowActivated, where we call the original handler at the beginning to stay compatible).

According to the call hierarchy part of the code search site, SetActivationChangeObserver is only called from NativeWidgetAura (which we don't disturb) and from DesktopNativeWidgetAura, which we correctly handle. I don't see any way that we could overwrite another observer here, because we only effectively overwrite with the same class instance.

// as it should be.
// TODO(brenca): Remove this fix when the chromium issue is fixed.
// crbug.com/724538
tooltip_controller->OnCancelMode(nullptr);

This comment has been minimized.

@ckerr

ckerr Jan 18, 2018

Member

Is there any way for wm::GetTooltipClient() to return nullptr such that this would be a null pointer dereference?

This comment has been minimized.

@brenca

brenca Jan 18, 2018

Author Member

That could return NULL, 'tho I don't really see when. Let's add an if just in case.

@ckerr

ckerr approved these changes Jan 18, 2018

@ckerr ckerr merged commit d0af17e into electron:master Jan 19, 2018

7 of 8 checks passed

continuous-integration/jenkins/pr-head This commit cannot be built
Details
ci/circleci: electron-linux-arm Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-ia32 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-x64 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment