From 3ea623364b5c3b03d803884f612016221d7b5695 Mon Sep 17 00:00:00 2001 From: Maksim Sisov Date: Wed, 22 Jan 2025 12:59:48 +0200 Subject: [PATCH] feat: support global shortcuts via GlobalShortcutsPortal feature with ozone/wayland (#45171) * fix: backport patch to fix systemd unit activation in Chromium This backports a patch from Chromium, which fixes systemd unit activation. That is, a globalShortcuts feature that Chromium has needs to create a systemd unit and rename it properly. Portal's global shortcuts uses that name afterwards to map the app with the shortcuts bound. However, there might be a race between Chromium binding shortcuts and renaming the unit. This is a first step to add Portal's globalShortcuts to Electron. * feat: Support global shortcuts via GlobalShortcutsPortal feature Chromium has a new feature called GlobalShortcutsPortal. It allows clients to use Portal's globalShortcuts to register and listen to shortcuts. This patches adds necessary bits, which allows Electron to use that feature. In order to make it work, one has to add --enable-features=GlobalShortcutsPortal Test: tested manually with a sample app. * docs: add GlobalShortcutsPortal feature to globalShortcuts docs Electron supports Portal's globalShortcuts API now via Chromium, and Electron apps can use that in a Wayland session. Update the docs with the required feature flag that must be passed to be able to use that implementation. --- docs/api/global-shortcut.md | 8 + patches/chromium/.patches | 1 + ...ivate_before_notifying_about_success.patch | 497 ++++++++++++++++++ .../api/electron_api_global_shortcut.cc | 81 ++- .../api/electron_api_global_shortcut.h | 2 + shell/browser/electron_browser_context.cc | 4 + shell/common/electron_constants.h | 7 + 7 files changed, 591 insertions(+), 9 deletions(-) create mode 100644 patches/chromium/check_for_unit_to_activate_before_notifying_about_success.patch diff --git a/docs/api/global-shortcut.md b/docs/api/global-shortcut.md index 9fe98c0415f0..142bdbaba1ad 100644 --- a/docs/api/global-shortcut.md +++ b/docs/api/global-shortcut.md @@ -12,9 +12,17 @@ shortcuts. not have the keyboard focus. This module cannot be used before the `ready` event of the app module is emitted. +Please also note that it is also possible to use Chromium's +`GlobalShortcutsPortal` implementation, which allows apps to bind global +shortcuts when running within a Wayland session. + ```js const { app, globalShortcut } = require('electron') +// Enable usage of Portal's globalShortcuts. This is essential for cases when +// the app runs in a Wayland session. +app.commandLine.appendSwitch('enable-features', 'GlobalShortcutsPortal') + app.whenReady().then(() => { // Register a 'CommandOrControl+X' shortcut listener. const ret = globalShortcut.register('CommandOrControl+X', () => { diff --git a/patches/chromium/.patches b/patches/chromium/.patches index 939c67537716..ba53247a1cc8 100644 --- a/patches/chromium/.patches +++ b/patches/chromium/.patches @@ -139,3 +139,4 @@ refactor_unfilter_unresponsive_events.patch build_disable_thin_lto_mac.patch build_add_public_config_simdutf_config.patch revert_code_health_clean_up_stale_macwebcontentsocclusion.patch +check_for_unit_to_activate_before_notifying_about_success.patch diff --git a/patches/chromium/check_for_unit_to_activate_before_notifying_about_success.patch b/patches/chromium/check_for_unit_to_activate_before_notifying_about_success.patch new file mode 100644 index 000000000000..6dacb5b1225f --- /dev/null +++ b/patches/chromium/check_for_unit_to_activate_before_notifying_about_success.patch @@ -0,0 +1,497 @@ +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 +From: Maksim Sisov +Date: Tue, 7 Jan 2025 23:46:56 -0800 +Subject: Check for unit to activate before notifying about success + +Portal's globalShortcuts interface relies on the unit name to +properly assign a client for the bound commands. However, in +some scenarious, there is a race between the service to be +created, changed its name and activated. As a result, shortcuts +might be bound before the name is changed. As a result, shortcuts +might not correctly work and the client will not receive any +signals. + +This is mostly not an issue for Chromium as it creates the +global shortcuts portal linux object way earlier than it gets +commands from the command service. But downstream project, which +try to bind shortcuts at the same time as that instance is created, +may experience this issue. As a result, they might not have +shortcuts working correctly after system reboot or app restart as +there is a race between those operations. + +Bug: None +Change-Id: I8346d65e051d9587850c76ca0b8807669c161667 +Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6110782 +Reviewed-by: Thomas Anderson +Commit-Queue: Maksim Sisov +Cr-Commit-Position: refs/heads/main@{#1403434} + +diff --git a/components/dbus/xdg/systemd.cc b/components/dbus/xdg/systemd.cc +index 362a16447bf578923cb8a84674c277ae6c98228f..3cd9a55d540c07a4c53f5a62bec5cbea37c11838 100644 +--- a/components/dbus/xdg/systemd.cc ++++ b/components/dbus/xdg/systemd.cc +@@ -4,9 +4,12 @@ + + #include "components/dbus/xdg/systemd.h" + ++#include + #include + + #include "base/environment.h" ++#include "base/functional/bind.h" ++#include "base/functional/callback_helpers.h" + #include "base/memory/scoped_refptr.h" + #include "base/no_destructor.h" + #include "base/sequence_checker.h" +@@ -17,7 +20,9 @@ + #include "components/dbus/utils/name_has_owner.h" + #include "dbus/bus.h" + #include "dbus/message.h" ++#include "dbus/object_path.h" + #include "dbus/object_proxy.h" ++#include "dbus/property.h" + #include "third_party/abseil-cpp/absl/types/variant.h" + + namespace dbus_xdg { +@@ -37,6 +42,10 @@ constexpr char kServiceNameSystemd[] = "org.freedesktop.systemd1"; + constexpr char kObjectPathSystemd[] = "/org/freedesktop/systemd1"; + constexpr char kInterfaceSystemdManager[] = "org.freedesktop.systemd1.Manager"; + constexpr char kMethodStartTransientUnit[] = "StartTransientUnit"; ++constexpr char kMethodGetUnit[] = "GetUnit"; ++ ++constexpr char kInterfaceSystemdUnit[] = "org.freedesktop.systemd1.Unit"; ++constexpr char kActiveStateProp[] = "ActiveState"; + + constexpr char kUnitNameFormat[] = "app-$1$2-$3.scope"; + +@@ -67,6 +76,81 @@ const char* GetAppNameSuffix(const std::string& channel) { + return ""; + } + ++// Declare this helper for SystemdUnitActiveStateWatcher to be used. ++void SetStateAndRunCallbacks(SystemdUnitStatus result); ++ ++// Watches the object to become active and fires callbacks via ++// SetStateAndRunCallbacks. The callbacks are fired whenever a response with the ++// state being "active" or "failed" (or similar) comes. ++// ++// PS firing callbacks results in destroying this object. So any references ++// to this become invalid. ++class SystemdUnitActiveStateWatcher : public dbus::PropertySet { ++ public: ++ SystemdUnitActiveStateWatcher(scoped_refptr bus, ++ dbus::ObjectProxy* object_proxy) ++ : dbus::PropertySet(object_proxy, ++ kInterfaceSystemdUnit, ++ base::BindRepeating( ++ &SystemdUnitActiveStateWatcher::OnPropertyChanged, ++ base::Unretained(this))), ++ bus_(bus) { ++ RegisterProperty(kActiveStateProp, &active_state_); ++ ConnectSignals(); ++ GetAll(); ++ } ++ ++ ~SystemdUnitActiveStateWatcher() override { ++ bus_->RemoveObjectProxy(kServiceNameSystemd, object_proxy()->object_path(), ++ base::DoNothing()); ++ } ++ ++ private: ++ void OnPropertyChanged(const std::string& property_name) { ++ DCHECK(active_state_.is_valid()); ++ const std::string state_value = active_state_.value(); ++ if (callbacks_called_ || state_value == "activating" || ++ state_value == "reloading") { ++ // Ignore if callbacks have already been fired or continue waiting until ++ // the state changes to something else. ++ return; ++ } ++ ++ // There are other states as failed, inactive, and deactivating. Treat all ++ // of them as failed. ++ callbacks_called_ = true; ++ SetStateAndRunCallbacks(state_value == "active" ++ ? SystemdUnitStatus::kUnitStarted ++ : SystemdUnitStatus::kFailedToStart); ++ MaybeDeleteSelf(); ++ } ++ ++ void OnGetAll(dbus::Response* response) override { ++ dbus::PropertySet::OnGetAll(response); ++ keep_alive_ = false; ++ MaybeDeleteSelf(); ++ } ++ ++ void MaybeDeleteSelf() { ++ if (!keep_alive_ && callbacks_called_) { ++ delete this; ++ } ++ } ++ ++ // Registered property that this listens updates to. ++ dbus::Property active_state_; ++ ++ // Indicates whether callbacks for the unit's state have been called. ++ bool callbacks_called_ = false; ++ ++ // Control variable that helps to defer the destruction of |this| as deleting ++ // self when the state changes to active during |OnGetAll| will result in a ++ // segfault. ++ bool keep_alive_ = true; ++ ++ scoped_refptr bus_; ++}; ++ + // Global state for cached result or pending callbacks. + StatusOrCallbacks& GetUnitNameState() { + static base::NoDestructor state( +@@ -83,10 +167,52 @@ void SetStateAndRunCallbacks(SystemdUnitStatus result) { + } + } + +-void OnStartTransientUnitResponse(dbus::Response* response) { ++void OnGetPathResponse(scoped_refptr bus, dbus::Response* response) { ++ dbus::MessageReader reader(response); ++ dbus::ObjectPath obj_path; ++ if (!response || !reader.PopObjectPath(&obj_path) || !obj_path.IsValid()) { ++ // We didn't get a valid response. Treat this as failed service. ++ SetStateAndRunCallbacks(SystemdUnitStatus::kFailedToStart); ++ return; ++ } ++ ++ dbus::ObjectProxy* unit_proxy = ++ bus->GetObjectProxy(kServiceNameSystemd, obj_path); ++ // Create the active state property watcher. It will destroy itself once ++ // it gets notified about the state change. ++ std::unique_ptr active_state_watcher = ++ std::make_unique(bus, unit_proxy); ++ active_state_watcher.release(); ++} ++ ++void WaitUnitActivateAndRunCallbacks(scoped_refptr bus, ++ std::string unit_name) { ++ // Get the path of the unit, which looks similar to ++ // /org/freedesktop/systemd1/unit/app_2dorg_2echromium_2eChromium_2d3182191_2escope ++ // and then wait for it activation. ++ dbus::ObjectProxy* systemd = bus->GetObjectProxy( ++ kServiceNameSystemd, dbus::ObjectPath(kObjectPathSystemd)); ++ ++ dbus::MethodCall method_call(kInterfaceSystemdManager, kMethodGetUnit); ++ dbus::MessageWriter writer(&method_call); ++ writer.AppendString(unit_name); ++ ++ systemd->CallMethod(&method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT, ++ base::BindOnce(&OnGetPathResponse, std::move(bus))); ++} ++ ++void OnStartTransientUnitResponse(scoped_refptr bus, ++ std::string unit_name, ++ dbus::Response* response) { + SystemdUnitStatus result = response ? SystemdUnitStatus::kUnitStarted + : SystemdUnitStatus::kFailedToStart; +- SetStateAndRunCallbacks(result); ++ // If the start of the unit failed, immediately notify the client. Otherwise, ++ // wait for its activation. ++ if (result == SystemdUnitStatus::kFailedToStart) { ++ SetStateAndRunCallbacks(result); ++ } else { ++ WaitUnitActivateAndRunCallbacks(std::move(bus), unit_name); ++ } + } + + void OnNameHasOwnerResponse(scoped_refptr bus, +@@ -128,8 +254,9 @@ void OnNameHasOwnerResponse(scoped_refptr bus, + properties.Write(&writer); + // No auxiliary units. + Dict().Write(&writer); +- systemd->CallMethod(&method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT, +- base::BindOnce(&OnStartTransientUnitResponse)); ++ systemd->CallMethod( ++ &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT, ++ base::BindOnce(&OnStartTransientUnitResponse, std::move(bus), unit_name)); + } + + } // namespace +diff --git a/components/dbus/xdg/systemd_unittest.cc b/components/dbus/xdg/systemd_unittest.cc +index 2e3baecabc4b479000c78d4f6bd30cb1f6e61d2e..67278d7033664d52fbbda02749a2aaa43352f402 100644 +--- a/components/dbus/xdg/systemd_unittest.cc ++++ b/components/dbus/xdg/systemd_unittest.cc +@@ -16,7 +16,9 @@ + #include "dbus/message.h" + #include "dbus/mock_bus.h" + #include "dbus/mock_object_proxy.h" ++#include "dbus/object_path.h" + #include "dbus/object_proxy.h" ++#include "dbus/property.h" + #include "testing/gmock/include/gmock/gmock.h" + #include "testing/gtest/include/gtest/gtest.h" + +@@ -32,6 +34,27 @@ constexpr char kServiceNameSystemd[] = "org.freedesktop.systemd1"; + constexpr char kObjectPathSystemd[] = "/org/freedesktop/systemd1"; + constexpr char kInterfaceSystemdManager[] = "org.freedesktop.systemd1.Manager"; + constexpr char kMethodStartTransientUnit[] = "StartTransientUnit"; ++constexpr char kMethodGetUnit[] = "GetUnit"; ++ ++constexpr char kFakeUnitPath[] = "/fake/unit/path"; ++constexpr char kActiveState[] = "ActiveState"; ++constexpr char kStateActive[] = "active"; ++constexpr char kStateInactive[] = "inactive"; ++ ++std::unique_ptr CreateActiveStateGetAllResponse( ++ const std::string& state) { ++ auto response = dbus::Response::CreateEmpty(); ++ dbus::MessageWriter writer(response.get()); ++ dbus::MessageWriter array_writer(nullptr); ++ dbus::MessageWriter dict_entry_writer(nullptr); ++ writer.OpenArray("{sv}", &array_writer); ++ array_writer.OpenDictEntry(&dict_entry_writer); ++ dict_entry_writer.AppendString(kActiveState); ++ dict_entry_writer.AppendVariantOfString(state); ++ array_writer.CloseContainer(&dict_entry_writer); ++ writer.CloseContainer(&array_writer); ++ return response; ++} + + class SetSystemdScopeUnitNameForXdgPortalTest : public ::testing::Test { + public: +@@ -124,17 +147,48 @@ TEST_F(SetSystemdScopeUnitNameForXdgPortalTest, StartTransientUnitSuccess) { + + EXPECT_CALL(*bus, GetObjectProxy(kServiceNameSystemd, + dbus::ObjectPath(kObjectPathSystemd))) +- .WillOnce(Return(mock_systemd_proxy.get())); ++ .Times(2) ++ .WillRepeatedly(Return(mock_systemd_proxy.get())); ++ ++ auto mock_dbus_unit_proxy = base::MakeRefCounted( ++ bus.get(), kServiceNameSystemd, dbus::ObjectPath(kFakeUnitPath)); ++ EXPECT_CALL(*bus, GetObjectProxy(kServiceNameSystemd, ++ dbus::ObjectPath(kFakeUnitPath))) ++ .WillOnce(Return(mock_dbus_unit_proxy.get())); + + EXPECT_CALL(*mock_systemd_proxy, DoCallMethod(_, _, _)) + .WillOnce(Invoke([](dbus::MethodCall* method_call, int timeout_ms, + dbus::ObjectProxy::ResponseCallback* callback) { ++ // Expect kMethodStartTransientUnit first. + EXPECT_EQ(method_call->GetInterface(), kInterfaceSystemdManager); + EXPECT_EQ(method_call->GetMember(), kMethodStartTransientUnit); + + // Simulate a successful response + auto response = dbus::Response::CreateEmpty(); + std::move(*callback).Run(response.get()); ++ })) ++ .WillOnce(Invoke([obj_path = kFakeUnitPath]( ++ dbus::MethodCall* method_call, int timeout_ms, ++ dbus::ObjectProxy::ResponseCallback* callback) { ++ // Then expect kMethodGetUnit. A valid path must be provided. ++ EXPECT_EQ(method_call->GetInterface(), kInterfaceSystemdManager); ++ EXPECT_EQ(method_call->GetMember(), kMethodGetUnit); ++ ++ // Simulate a successful response and provide a fake path to the object. ++ auto response = dbus::Response::CreateEmpty(); ++ dbus::MessageWriter writer(response.get()); ++ writer.AppendObjectPath(dbus::ObjectPath(obj_path)); ++ std::move(*callback).Run(response.get()); ++ })); ++ ++ EXPECT_CALL(*mock_dbus_unit_proxy, DoCallMethod(_, _, _)) ++ .WillOnce(Invoke([](dbus::MethodCall* method_call, int timeout_ms, ++ dbus::ObjectProxy::ResponseCallback* callback) { ++ EXPECT_EQ(method_call->GetInterface(), dbus::kPropertiesInterface); ++ EXPECT_EQ(method_call->GetMember(), dbus::kPropertiesGetAll); ++ // Simulate a successful response with "active" state. ++ auto response = CreateActiveStateGetAllResponse(kStateActive); ++ std::move(*callback).Run(response.get()); + })); + + std::optional status; +@@ -189,6 +243,142 @@ TEST_F(SetSystemdScopeUnitNameForXdgPortalTest, StartTransientUnitFailure) { + EXPECT_EQ(status, SystemdUnitStatus::kFailedToStart); + } + ++TEST_F(SetSystemdScopeUnitNameForXdgPortalTest, ++ StartTransientUnitInvalidUnitPath) { ++ scoped_refptr bus = ++ base::MakeRefCounted(dbus::Bus::Options()); ++ ++ auto mock_dbus_proxy = base::MakeRefCounted( ++ bus.get(), DBUS_SERVICE_DBUS, dbus::ObjectPath(DBUS_PATH_DBUS)); ++ ++ EXPECT_CALL( ++ *bus, GetObjectProxy(DBUS_SERVICE_DBUS, dbus::ObjectPath(DBUS_PATH_DBUS))) ++ .WillRepeatedly(Return(mock_dbus_proxy.get())); ++ ++ EXPECT_CALL(*mock_dbus_proxy, DoCallMethod(_, _, _)) ++ .WillOnce(Invoke([](dbus::MethodCall* method_call, int timeout_ms, ++ dbus::ObjectProxy::ResponseCallback* callback) { ++ auto response = dbus::Response::CreateEmpty(); ++ dbus::MessageWriter writer(response.get()); ++ writer.AppendBool(true); ++ std::move(*callback).Run(response.get()); ++ })); ++ ++ auto mock_systemd_proxy = base::MakeRefCounted( ++ bus.get(), kServiceNameSystemd, dbus::ObjectPath(kObjectPathSystemd)); ++ ++ EXPECT_CALL(*bus, GetObjectProxy(kServiceNameSystemd, ++ dbus::ObjectPath(kObjectPathSystemd))) ++ .Times(2) ++ .WillRepeatedly(Return(mock_systemd_proxy.get())); ++ ++ EXPECT_CALL(*mock_systemd_proxy, DoCallMethod(_, _, _)) ++ .WillOnce(Invoke([](dbus::MethodCall* method_call, int timeout_ms, ++ dbus::ObjectProxy::ResponseCallback* callback) { ++ EXPECT_EQ(method_call->GetInterface(), kInterfaceSystemdManager); ++ EXPECT_EQ(method_call->GetMember(), kMethodStartTransientUnit); ++ ++ // Simulate a successful response ++ auto response = dbus::Response::CreateEmpty(); ++ std::move(*callback).Run(response.get()); ++ })) ++ .WillOnce(Invoke([](dbus::MethodCall* method_call, int timeout_ms, ++ dbus::ObjectProxy::ResponseCallback* callback) { ++ EXPECT_EQ(method_call->GetInterface(), kInterfaceSystemdManager); ++ EXPECT_EQ(method_call->GetMember(), kMethodGetUnit); ++ ++ // Simulate a failure response. ++ std::move(*callback).Run(nullptr); ++ })); ++ ++ std::optional status; ++ ++ SetSystemdScopeUnitNameForXdgPortal( ++ bus.get(), base::BindLambdaForTesting( ++ [&](SystemdUnitStatus result) { status = result; })); ++ ++ EXPECT_EQ(status, SystemdUnitStatus::kFailedToStart); ++} ++ ++TEST_F(SetSystemdScopeUnitNameForXdgPortalTest, ++ StartTransientUnitFailToActivate) { ++ scoped_refptr bus = ++ base::MakeRefCounted(dbus::Bus::Options()); ++ ++ auto mock_dbus_proxy = base::MakeRefCounted( ++ bus.get(), DBUS_SERVICE_DBUS, dbus::ObjectPath(DBUS_PATH_DBUS)); ++ ++ EXPECT_CALL( ++ *bus, GetObjectProxy(DBUS_SERVICE_DBUS, dbus::ObjectPath(DBUS_PATH_DBUS))) ++ .WillRepeatedly(Return(mock_dbus_proxy.get())); ++ ++ EXPECT_CALL(*mock_dbus_proxy, DoCallMethod(_, _, _)) ++ .WillOnce(Invoke([](dbus::MethodCall* method_call, int timeout_ms, ++ dbus::ObjectProxy::ResponseCallback* callback) { ++ auto response = dbus::Response::CreateEmpty(); ++ dbus::MessageWriter writer(response.get()); ++ writer.AppendBool(true); ++ std::move(*callback).Run(response.get()); ++ })); ++ ++ auto mock_systemd_proxy = base::MakeRefCounted( ++ bus.get(), kServiceNameSystemd, dbus::ObjectPath(kObjectPathSystemd)); ++ ++ EXPECT_CALL(*bus, GetObjectProxy(kServiceNameSystemd, ++ dbus::ObjectPath(kObjectPathSystemd))) ++ .Times(2) ++ .WillRepeatedly(Return(mock_systemd_proxy.get())); ++ ++ auto mock_dbus_unit_proxy = base::MakeRefCounted( ++ bus.get(), kServiceNameSystemd, dbus::ObjectPath(kFakeUnitPath)); ++ EXPECT_CALL(*bus, GetObjectProxy(kServiceNameSystemd, ++ dbus::ObjectPath(kFakeUnitPath))) ++ .WillOnce(Return(mock_dbus_unit_proxy.get())); ++ ++ EXPECT_CALL(*mock_systemd_proxy, DoCallMethod(_, _, _)) ++ .WillOnce(Invoke([](dbus::MethodCall* method_call, int timeout_ms, ++ dbus::ObjectProxy::ResponseCallback* callback) { ++ EXPECT_EQ(method_call->GetInterface(), kInterfaceSystemdManager); ++ EXPECT_EQ(method_call->GetMember(), kMethodStartTransientUnit); ++ ++ // Simulate a successful response ++ auto response = dbus::Response::CreateEmpty(); ++ std::move(*callback).Run(response.get()); ++ })) ++ .WillOnce(Invoke([obj_path = kFakeUnitPath]( ++ dbus::MethodCall* method_call, int timeout_ms, ++ dbus::ObjectProxy::ResponseCallback* callback) { ++ EXPECT_EQ(method_call->GetInterface(), kInterfaceSystemdManager); ++ EXPECT_EQ(method_call->GetMember(), kMethodGetUnit); ++ ++ // Simulate a successful response ++ auto response = dbus::Response::CreateEmpty(); ++ dbus::MessageWriter writer(response.get()); ++ writer.AppendObjectPath(dbus::ObjectPath(obj_path)); ++ std::move(*callback).Run(response.get()); ++ })); ++ ++ EXPECT_CALL(*mock_dbus_unit_proxy, DoCallMethod(_, _, _)) ++ .WillOnce(Invoke([](dbus::MethodCall* method_call, int timeout_ms, ++ dbus::ObjectProxy::ResponseCallback* callback) { ++ // Then expect kMethodGetUnit. A valid path must be provided. ++ EXPECT_EQ(method_call->GetInterface(), dbus::kPropertiesInterface); ++ EXPECT_EQ(method_call->GetMember(), dbus::kPropertiesGetAll); ++ ++ // Simulate a successful response, but with inactive state. ++ auto response = CreateActiveStateGetAllResponse(kStateInactive); ++ std::move(*callback).Run(response.get()); ++ })); ++ ++ std::optional status; ++ ++ SetSystemdScopeUnitNameForXdgPortal( ++ bus.get(), base::BindLambdaForTesting( ++ [&](SystemdUnitStatus result) { status = result; })); ++ ++ EXPECT_EQ(status, SystemdUnitStatus::kFailedToStart); ++} ++ + TEST_F(SetSystemdScopeUnitNameForXdgPortalTest, UnitNameConstruction) { + scoped_refptr bus = + base::MakeRefCounted(dbus::Bus::Options()); +@@ -220,7 +410,14 @@ TEST_F(SetSystemdScopeUnitNameForXdgPortalTest, UnitNameConstruction) { + + EXPECT_CALL(*bus, GetObjectProxy(kServiceNameSystemd, + dbus::ObjectPath(kObjectPathSystemd))) +- .WillOnce(Return(mock_systemd_proxy.get())); ++ .Times(2) ++ .WillRepeatedly(Return(mock_systemd_proxy.get())); ++ ++ auto mock_dbus_unit_proxy = base::MakeRefCounted( ++ bus.get(), kServiceNameSystemd, dbus::ObjectPath(kFakeUnitPath)); ++ EXPECT_CALL(*bus, GetObjectProxy(kServiceNameSystemd, ++ dbus::ObjectPath(kFakeUnitPath))) ++ .WillOnce(Return(mock_dbus_unit_proxy.get())); + + EXPECT_CALL(*mock_systemd_proxy, DoCallMethod(_, _, _)) + .WillOnce(Invoke([&](dbus::MethodCall* method_call, int timeout_ms, +@@ -256,6 +453,30 @@ TEST_F(SetSystemdScopeUnitNameForXdgPortalTest, UnitNameConstruction) { + + auto response = dbus::Response::CreateEmpty(); + std::move(*callback).Run(response.get()); ++ })) ++ .WillOnce(Invoke([obj_path = kFakeUnitPath]( ++ dbus::MethodCall* method_call, int timeout_ms, ++ dbus::ObjectProxy::ResponseCallback* callback) { ++ EXPECT_EQ(method_call->GetInterface(), kInterfaceSystemdManager); ++ EXPECT_EQ(method_call->GetMember(), kMethodGetUnit); ++ ++ // Simulate a successful response ++ auto response = dbus::Response::CreateEmpty(); ++ dbus::MessageWriter writer(response.get()); ++ writer.AppendObjectPath(dbus::ObjectPath(obj_path)); ++ std::move(*callback).Run(response.get()); ++ })); ++ ++ EXPECT_CALL(*mock_dbus_unit_proxy, DoCallMethod(_, _, _)) ++ .WillOnce(Invoke([](dbus::MethodCall* method_call, int timeout_ms, ++ dbus::ObjectProxy::ResponseCallback* callback) { ++ // Then expect kMethodGetUnit. A valid path must be provided. ++ EXPECT_EQ(method_call->GetInterface(), dbus::kPropertiesInterface); ++ EXPECT_EQ(method_call->GetMember(), dbus::kPropertiesGetAll); ++ ++ // Simulate a successful response ++ auto response = CreateActiveStateGetAllResponse(kStateActive); ++ std::move(*callback).Run(response.get()); + })); + + std::optional status; diff --git a/shell/browser/api/electron_api_global_shortcut.cc b/shell/browser/api/electron_api_global_shortcut.cc index f0b973ce7725..319f5436387d 100644 --- a/shell/browser/api/electron_api_global_shortcut.cc +++ b/shell/browser/api/electron_api_global_shortcut.cc @@ -4,9 +4,15 @@ #include "shell/browser/api/electron_api_global_shortcut.h" +#include #include #include "base/containers/contains.h" +#include "base/strings/utf_string_conversions.h" +#include "base/uuid.h" +#include "components/prefs/pref_service.h" +#include "electron/shell/browser/electron_browser_context.h" +#include "electron/shell/common/electron_constants.h" #include "extensions/common/command.h" #include "gin/dictionary.h" #include "gin/handle.h" @@ -62,7 +68,12 @@ void GlobalShortcut::OnKeyPressed(const ui::Accelerator& accelerator) { void GlobalShortcut::ExecuteCommand(const extensions::ExtensionId& extension_id, const std::string& command_id) { - // Ignore extension commands + if (!base::Contains(command_callback_map_, command_id)) { + // This should never occur, because if it does, GlobalShortcutListener + // notifies us with wrong command. + NOTREACHED(); + } + command_callback_map_[command_id].Run(); } bool GlobalShortcut::RegisterAll( @@ -103,13 +114,56 @@ bool GlobalShortcut::Register(const ui::Accelerator& accelerator, } #endif - if (!GlobalShortcutListener::GetInstance()->RegisterAccelerator(accelerator, - this)) { + auto* instance = GlobalShortcutListener::GetInstance(); + if (!instance) { return false; } - accelerator_callback_map_[accelerator] = callback; - return true; + if (instance->IsRegistrationHandledExternally()) { + auto* context = ElectronBrowserContext::From("", false); + PrefService* prefs = context->prefs(); + + // Need a unique profile id. Set one if not generated yet, otherwise re-use + // the same so that the session for the globalShortcuts is able to get + // already registered shortcuts from the previous session. This will be used + // by GlobalShortcutListenerLinux as a session key. + std::string profile_id = prefs->GetString(kElectronGlobalShortcutsUuid); + if (profile_id.empty()) { + profile_id = base::Uuid::GenerateRandomV4().AsLowercaseString(); + prefs->SetString(kElectronGlobalShortcutsUuid, profile_id); + } + + // There is no way to get command id for the accelerator as it's extensions' + // thing. Instead, we can convert it to string in a following example form + // - std::string("Alt+Shift+K"). That must be sufficient enough for us to + // map this accelerator with registered commands. + const std::string command_str = + extensions::Command::AcceleratorToString(accelerator); + ui::CommandMap commands; + extensions::Command command( + command_str, base::UTF8ToUTF16("Electron shortcut " + command_str), + /*accelerator=*/std::string(), /*global=*/true); + command.set_accelerator(accelerator); + commands[command_str] = command; + + // In order to distinguish the shortcuts, we must register multiple commands + // as different extensions. Otherwise, each shortcut will be an alternative + // for the very first registered and we'll not be able to distinguish them. + // For example, if Alt+Shift+K is registered first, registering and pressing + // Alt+Shift+M will trigger global shortcuts, but the command id that is + // received by GlobalShortcut will correspond to Alt+Shift+K as our command + // id is basically a stringified accelerator. + const std::string fake_extension_id = command_str + "+" + profile_id; + instance->OnCommandsChanged(fake_extension_id, profile_id, commands, this); + command_callback_map_[command_str] = callback; + return true; + } else { + if (instance->RegisterAccelerator(accelerator, this)) { + accelerator_callback_map_[accelerator] = callback; + return true; + } + } + return false; } void GlobalShortcut::Unregister(const ui::Accelerator& accelerator) { @@ -127,8 +181,10 @@ void GlobalShortcut::Unregister(const ui::Accelerator& accelerator) { } #endif - GlobalShortcutListener::GetInstance()->UnregisterAccelerator(accelerator, - this); + if (GlobalShortcutListener::GetInstance()) { + GlobalShortcutListener::GetInstance()->UnregisterAccelerator(accelerator, + this); + } } void GlobalShortcut::UnregisterSome( @@ -139,7 +195,12 @@ void GlobalShortcut::UnregisterSome( } bool GlobalShortcut::IsRegistered(const ui::Accelerator& accelerator) { - return base::Contains(accelerator_callback_map_, accelerator); + if (base::Contains(accelerator_callback_map_, accelerator)) { + return true; + } + const std::string command_str = + extensions::Command::AcceleratorToString(accelerator); + return base::Contains(command_callback_map_, command_str); } void GlobalShortcut::UnregisterAll() { @@ -149,7 +210,9 @@ void GlobalShortcut::UnregisterAll() { return; } accelerator_callback_map_.clear(); - GlobalShortcutListener::GetInstance()->UnregisterAccelerators(this); + if (GlobalShortcutListener::GetInstance()) { + GlobalShortcutListener::GetInstance()->UnregisterAccelerators(this); + } } // static diff --git a/shell/browser/api/electron_api_global_shortcut.h b/shell/browser/api/electron_api_global_shortcut.h index 1f0de597f3b6..e35ee691fab2 100644 --- a/shell/browser/api/electron_api_global_shortcut.h +++ b/shell/browser/api/electron_api_global_shortcut.h @@ -44,6 +44,7 @@ class GlobalShortcut final private: typedef std::map AcceleratorCallbackMap; + typedef std::map CommandCallbackMap; bool RegisterAll(const std::vector& accelerators, const base::RepeatingClosure& callback); @@ -60,6 +61,7 @@ class GlobalShortcut final const std::string& command_id) override; AcceleratorCallbackMap accelerator_callback_map_; + CommandCallbackMap command_callback_map_; }; } // namespace electron::api diff --git a/shell/browser/electron_browser_context.cc b/shell/browser/electron_browser_context.cc index e1f685a79907..4ad77d32b217 100644 --- a/shell/browser/electron_browser_context.cc +++ b/shell/browser/electron_browser_context.cc @@ -458,6 +458,10 @@ void ElectronBrowserContext::InitPrefs() { } } #endif + + // Unique uuid for global shortcuts. + registry->RegisterStringPref(electron::kElectronGlobalShortcutsUuid, + std::string()); } void ElectronBrowserContext::SetUserAgent(const std::string& user_agent) { diff --git a/shell/common/electron_constants.h b/shell/common/electron_constants.h index 1050533c7270..2ddf98d25acf 100644 --- a/shell/common/electron_constants.h +++ b/shell/common/electron_constants.h @@ -23,6 +23,13 @@ inline constexpr std::string_view kDeviceSerialNumberKey = "serialNumber"; inline constexpr base::cstring_view kRunAsNode = "ELECTRON_RUN_AS_NODE"; +// Per-profile UUID to distinguish global shortcut sessions for +// org.freedesktop.portal.GlobalShortcuts. This is a counterpart to +// extensions::pref_names::kGlobalShortcutsUuid, which may be not defined +// if extensions are disabled. +inline constexpr char kElectronGlobalShortcutsUuid[] = + "electron.global_shortcuts.uuid"; + #if BUILDFLAG(ENABLE_PDF_VIEWER) inline constexpr std::string_view kPDFExtensionPluginName = "Chromium PDF Viewer";