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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cache libnotify server caps #11965

merged 3 commits into from Feb 20, 2018


None yet
2 participants

ckerr commented Feb 19, 2018

An offshoot of the other libnotify branch.

Before this patch, the implementation of HasCapability() makes a blocking round-trip dbus call every time it's called, even though notify server capabilities are static and can be remembered & reused. This method is also called twice by LibnotifyNotification::Show(), there's a redundant blocking bus call before the first notification is ever shown.

In addition, NotifierSupportsActions() tries to cache whether or not actions is a supported capability; but due to a coding error, the cache flag was never set and so it also made a blocking round trip every time.

This patch caches the server capabilities.

Unrelated but also useful: this patch logs a warning if LibnotifyNotification::Initialize() fails. This appears to be the reason (well, one of the reasons) that the other libnotify branch is failing CI. 馃檮

@ckerr ckerr requested a review from electron/notifications as a code owner Feb 19, 2018

@ckerr ckerr merged commit 2a16b28 into master Feb 20, 2018

10 checks passed

WIP ready for review
ci/circleci: electron-linux-arm Your tests passed on CircleCI!
ci/circleci: electron-linux-arm-test Your tests passed on CircleCI!
ci/circleci: electron-linux-arm64 Your tests passed on CircleCI!
ci/circleci: electron-linux-arm64-test Your tests passed on CircleCI!
ci/circleci: electron-linux-ia32 Your tests passed on CircleCI!
ci/circleci: electron-linux-x64 Your tests passed on CircleCI!
continuous-integration/appveyor/branch AppVeyor build succeeded
continuous-integration/appveyor/pr AppVeyor build succeeded
continuous-integration/jenkins/pr-head This commit looks good

@ckerr ckerr deleted the cache-libnotify-server-caps branch Feb 20, 2018

sethlu added a commit to sethlu/electron that referenced this pull request May 3, 2018

Cache libnotify server caps (#11965)
* cache libnotify server capabilities

* fix broken production cache in NotifierSupportsActions()

* log a warning if LibnotifyNotification::Initialize() fails
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment