Closed
Bug 1360566
Opened 8 years ago
Closed 7 years ago
[Wayland] - Implement XRemote Server by D-Bus
Categories
(Core Graveyard :: X-remote, enhancement)
Tracking
(firefox59 fixed)
RESOLVED
FIXED
mozilla59
Tracking | Status | |
---|---|---|
firefox59 | --- | fixed |
People
(Reporter: stransky, Assigned: stransky)
References
Details
Attachments
(2 files)
XRemote does not work on Wayland now and needs to be disabled. We may replace the X11 backend here with DBus. This bug implements server side of XRemote DBus service.
Assignee | ||
Comment 1•8 years ago
|
||
Remote server implementation.
When running on non-X11 server it opens D-Bus service at org.mozilla.firefox.PROFILE_NAME (at nsGTKRemoteService::Connect()) and expose org.mozilla.firefox interface, /org/mozilla/Firefox/Remote object with OpenURL() method mapped to nsGTKRemoteService::OpenURL() (that's done by org.freedesktop.DBus.Introspectable object here).
A question is how to correctly implement nsGTKRemoteService::OpenURL() - unlike the X-remote we're missing XWindow and timestamp information.
Attachment #8862862 -
Flags: feedback?(karlt)
Comment 2•8 years ago
|
||
Comment on attachment 8862862 [details] [diff] [review]
remote_service.patch
(In reply to Martin Stránský from comment #1)
> When running on non-X11 server it opens D-Bus service at
> org.mozilla.firefox.PROFILE_NAME (at nsGTKRemoteService::Connect()) and
PROFILE_NAME can be arbitrary, so this might cause conflicts if a firefox
service of a different name org.mozilla.firefox.SERVICE2 is required one day.
Is that likely? Or would it just be another interface on the same service as
Remote?
> expose org.mozilla.firefox interface, /org/mozilla/Firefox/Remote object
> with OpenURL() method mapped to nsGTKRemoteService::OpenURL() (that's done
> by org.freedesktop.DBus.Introspectable object here).
>
> A question is how to correctly implement nsGTKRemoteService::OpenURL() -
> unlike the X-remote we're missing XWindow and timestamp information.
gdk_wayland_window_focus() does make use of the timestamp, and yet I don't see
an equivalent to gdk_x11_display_get_user_time() for Wayland and
gdk_wayland_window_set_startup_id() is a no-op, and so I don't know how to
extract an appropriate timestamp for Wayland.
Could Jan review this, please?
Attachment #8862862 -
Flags: feedback?(karlt) → feedback+
Comment 3•8 years ago
|
||
Comment on attachment 8862862 [details] [diff] [review]
remote_service.patch
Review of attachment 8862862 [details] [diff] [review]:
-----------------------------------------------------------------
::: toolkit/components/remote/nsGTKRemoteService.cpp
@@ +176,5 @@
> +{
> + HandleCommandLine(aCommandLine, nullptr, 0);
> +}
> +
> +#define MOZILLA_REMOTE_OBJECT "/org/mozilla/Firefox/Remote"
Firefox shouldn't be hardcoded in here and anywhere in the dbus interface. Because not everything using this code is called Firefox. Think e.g. Thunderbird, and things running firefox -app.
Attachment #8862862 -
Flags: feedback-
Assignee | ||
Comment 4•8 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #3)
> Comment on attachment 8862862 [details] [diff] [review]
> remote_service.patch
>
> Review of attachment 8862862 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: toolkit/components/remote/nsGTKRemoteService.cpp
> @@ +176,5 @@
> > +{
> > + HandleCommandLine(aCommandLine, nullptr, 0);
> > +}
> > +
> > +#define MOZILLA_REMOTE_OBJECT "/org/mozilla/Firefox/Remote"
>
> Firefox shouldn't be hardcoded in here and anywhere in the dbus interface.
> Because not everything using this code is called Firefox. Think e.g.
> Thunderbird, and things running firefox -app.
Sure, no big deal here. The object is exported as child of the dbus interface object and we can use some generic name like /org/mozilla/Remote or set app name runtime.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → stransky
Comment hidden (mozreview-request) |
Comment 6•7 years ago
|
||
mozreview-review |
Comment on attachment 8919682 [details]
Bug 1360566 - [Wayland] - Implement XRemote Server by D-Bus,
https://reviewboard.mozilla.org/r/190596/#review196320
I rather to see remote dbus implementation in another class derived from nsXRemoteService or even nsIRemoteService.
That would mean to modify code also there: http://searchfox.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp#4638
You should also consider not using nsXRemoteService::HandleCommandLine because it's hardly working in Wayland session (see https://mail.gnome.org/archives/commits-list/2016-November/msg02837.html) and make own implementation instead.
::: toolkit/components/remote/nsGTKRemoteService.h:79
(Diff revision 1)
> nsInterfaceHashtable<nsPtrHashKey<GtkWidget>, nsIWeakReference> mWindows;
> GtkWidget* mServerWindow;
> + bool mIsDBusRemote;
> +#ifdef ENABLE_REMOTE_DBUS
> + RefPtr<DBusConnection> mConnection;
> + char* mAppName;
Could you please use nsCString instead of char*? That avoids need for releasing string later.
::: toolkit/components/remote/nsGTKRemoteService.cpp:230
(Diff revision 1)
> +
> + return DBUS_HANDLER_RESULT_HANDLED;
> +}
> +
> +DBusHandlerResult
> +nsGTKRemoteService::OpenURL(DBusMessage *msg)
Please avoid overloading of the methods, use OpenURLMethodHandler or something different from OpenURL
::: toolkit/components/remote/nsGTKRemoteService.cpp:317
(Diff revision 1)
> + if (!mConnection)
> + return false;
> +
> + dbus_connection_set_exit_on_disconnect(mConnection, false);
> +
> + if (!aProfileName || aProfileName[0] == '\0') {
Is this required? If so, I see a problem with dev-edition default profile name:
http://searchfox.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp#2609
Maybe assert could help there instead and get rid of DEFAULT_PROFILE_NAME completely.
Attachment #8919682 -
Flags: review?(jhorak) → review-
Assignee | ||
Comment 7•7 years ago
|
||
mozreview-review |
Comment on attachment 8919682 [details]
Bug 1360566 - [Wayland] - Implement XRemote Server by D-Bus,
https://reviewboard.mozilla.org/r/190596/#review203300
::: toolkit/components/remote/nsGTKRemoteService.cpp:317
(Diff revision 1)
> + if (!mConnection)
> + return false;
> +
> + dbus_connection_set_exit_on_disconnect(mConnection, false);
> +
> + if (!aProfileName || aProfileName[0] == '\0') {
Moved to upper level.
Comment hidden (mozreview-request) |
Comment 9•7 years ago
|
||
mozreview-review |
Comment on attachment 8919682 [details]
Bug 1360566 - [Wayland] - Implement XRemote Server by D-Bus,
https://reviewboard.mozilla.org/r/190596/#review203310
C/C++ static analysis found 6 defects in this patch.
You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`
::: toolkit/components/remote/nsDBusRemoteService.h:24
(Diff revision 2)
> +public:
> + // We will be a static singleton, so don't use the ordinary methods.
> + NS_DECL_ISUPPORTS
> + NS_DECL_NSIREMOTESERVICE
> +
> + nsDBusRemoteService(nsRemoteService* aParentRemoteService)
Error: Bad implicit conversion constructor for 'nsdbusremoteservice' [clang-tidy: mozilla-implicit-constructor]
::: toolkit/components/remote/nsDBusRemoteService.h:24
(Diff revision 2)
> +public:
> + // We will be a static singleton, so don't use the ordinary methods.
> + NS_DECL_ISUPPORTS
> + NS_DECL_NSIREMOTESERVICE
> +
> + nsDBusRemoteService(nsRemoteService* aParentRemoteService)
Error: Bad implicit conversion constructor for 'nsdbusremoteservice' [clang-tidy: mozilla-implicit-constructor]
::: toolkit/components/remote/nsDBusRemoteService.cpp:97
(Diff revision 2)
> + const char *message = introspect_xml.get();
> + dbus_message_append_args(reply,
> + DBUS_TYPE_STRING, &message,
> + DBUS_TYPE_INVALID);
> +
> + dbus_connection_send(mConnection, reply, NULL);
Warning: Use nullptr [clang-tidy: modernize-use-nullptr]
dbus_connection_send(mConnection, reply, NULL);
^
nullptr
::: toolkit/components/remote/nsDBusRemoteService.cpp:120
(Diff revision 2)
> + } else {
> + mParentRemoteService->HandleCommandLine(commandLine, nullptr, 0);
> + reply = dbus_message_new_method_return(msg);
> + }
> +
> + dbus_connection_send(mConnection, reply, NULL);
Warning: Use nullptr [clang-tidy: modernize-use-nullptr]
dbus_connection_send(mConnection, reply, NULL);
^
nullptr
::: toolkit/components/remote/nsGTKRemoteService.h:26
(Diff revision 2)
> public:
> - // We will be a static singleton, so don't use the ordinary methods.
> NS_DECL_ISUPPORTS
> NS_DECL_NSIREMOTESERVICE
>
> + nsGTKRemoteService(nsRemoteService* aParentRemoteService)
Error: Bad implicit conversion constructor for 'nsgtkremoteservice' [clang-tidy: mozilla-implicit-constructor]
::: toolkit/components/remote/nsGTKRemoteService.h:26
(Diff revision 2)
> public:
> - // We will be a static singleton, so don't use the ordinary methods.
> NS_DECL_ISUPPORTS
> NS_DECL_NSIREMOTESERVICE
>
> + nsGTKRemoteService(nsRemoteService* aParentRemoteService)
Error: Bad implicit conversion constructor for 'nsgtkremoteservice' [clang-tidy: mozilla-implicit-constructor]
Comment 10•7 years ago
|
||
mozreview-review |
Comment on attachment 8919682 [details]
Bug 1360566 - [Wayland] - Implement XRemote Server by D-Bus,
https://reviewboard.mozilla.org/r/190596/#review204442
::: toolkit/components/remote/nsDBusRemoteService.h:15
(Diff revision 2)
> +
> +#include "nsIRemoteService.h"
> +#include "mozilla/DBusHelpers.h"
> +#include "nsString.h"
> +
> +class nsRemoteService;
When nsRemoteService calls used as static, no need to add this there.
::: toolkit/components/remote/nsDBusRemoteService.h:45
(Diff revision 2)
> +
> + // The connection is owned by DBus library
> + RefPtr<DBusConnection> mConnection;
> + nsCString mAppName;
> + // We're owned by this nsRemoteService instance
> + nsRemoteService* mParentRemoteService;
Don't add nsRemoteService as member.
::: toolkit/components/remote/nsDBusRemoteService.cpp:44
(Diff revision 2)
> + return NS_ERROR_ALREADY_INITIALIZED;
> +
> + mAppName = aAppName;
> + ToLowerCase(mAppName);
> +
> + if (!Connect(aAppName, aProfileName))
maybe nit: if you are not calling Connect from another place, you can put its content there. But I'll leave up to you.
::: toolkit/components/remote/nsDBusRemoteService.cpp:61
(Diff revision 2)
> +
> +NS_IMETHODIMP
> +nsDBusRemoteService::RegisterWindow(mozIDOMWindow* aWindow)
> +{
> + // We don't listen for property change events on DBus remote
> + return NS_OK;
Maybe better to return NS_ERROR_NOT_IMPLEMENTED?
::: toolkit/components/remote/nsDBusRemoteService.cpp:116
(Diff revision 2)
> + &commandLine, &length, DBUS_TYPE_INVALID) || length == 0) {
> + nsAutoCString errorMsg;
> + errorMsg = nsPrintfCString("org.mozilla.%s.Error", mAppName.get());
> + reply = dbus_message_new_error(msg, errorMsg.get(), "Wrong argument");
> + } else {
> + mParentRemoteService->HandleCommandLine(commandLine, nullptr, 0);
Use nsRemoteService::HandleCommandLine now.
::: toolkit/components/remote/nsDBusRemoteService.cpp:198
(Diff revision 2)
> + if (ret == -1) {
> + dbus_connection_unref(mConnection);
> + mConnection = nullptr;
> + return false;
> + }
> +
> + nsAutoCString objectName;
> + objectName = nsPrintfCString("/org/mozilla/%s/Remote", aAppName);
> +
> + if (!dbus_connection_register_object_path(mConnection, objectName.get(),
> + &remoteHandlersTable, this)) {
> + dbus_connection_unref(mConnection);
> + mConnection = nullptr;
> + return false;
> + }
please fix indentation. double space character instead of one.
::: toolkit/components/remote/nsDBusRemoteService.cpp:204
(Diff revision 2)
> + nsAutoCString objectName;
> + objectName = nsPrintfCString("/org/mozilla/%s/Remote", aAppName);
nit: can stay in one line.
::: toolkit/components/remote/nsDBusRemoteService.cpp:209
(Diff revision 2)
> + dbus_connection_unref(mConnection);
> + mConnection = nullptr;
Please do doublecheck the unrefing of the mConnection. In the ::Shutdown, you've mentioned it is not require to unref, but there it is done.
::: toolkit/components/remote/nsGTKRemoteService.h:32
(Diff revision 2)
> + : mServerWindow(nullptr)
> + , mParentRemoteService(aParentRemoteService)
> + { }
>
> - nsGTKRemoteService() :
> - mServerWindow(nullptr) { }
> + // Used for callback calls from Gtk+
> + static class nsGTKRemoteService* mActiveGTKRemoteService;
if static member, better to rename to sActiveGtkRemoteService
::: toolkit/components/remote/nsGTKRemoteService.h:39
(Diff revision 2)
> + gboolean HandlePropertyChange(GtkWidget *widget,
> + GdkEventProperty *event,
> + nsIWeakReference* aThis);
> +
> +protected:
> + virtual const char* HandleCommandLine(char* aBuffer, nsIDOMWindow* aWindow,
There should not be need for that anymore, or do I miss something?
::: toolkit/components/remote/nsGTKRemoteService.h:51
(Diff revision 2)
> - uint32_t aTimestamp) override;
> -
> nsInterfaceHashtable<nsPtrHashKey<GtkWidget>, nsIWeakReference> mWindows;
> - GtkWidget* mServerWindow;
> + GtkWidget* mServerWindow;
> + // We're owned by this nsRemoteService instance
> + nsRemoteService* mParentRemoteService;
as above with nsDBusService.
::: toolkit/components/remote/nsGTKRemoteService.cpp:99
(Diff revision 2)
> gtk_widget_destroy(mServerWindow);
> mServerWindow = nullptr;
> - return NS_OK;
> -}
>
> -// Set desktop startup ID to the passed ID, if there is one, so that any created
> + mActiveGTKRemoteService = nullptr;
Is it worth to move that on the very beginning of ::Shutdown() in case that early return to be sure that the mActiveGTKRemoteService won't stay initialized to some already freed pointer?
::: toolkit/components/remote/nsGTKRemoteService.cpp:105
(Diff revision 2)
> - if (!aDesktopStartupID.IsEmpty()) {
> + return NS_OK;
> - toolkit->SetDesktopStartupID(aDesktopStartupID);
> - }
> +}
>
> - toolkit->SetFocusTimestamp(aTimestamp);
> +const char*
> +nsGTKRemoteService::HandleCommandLine(char* aBuffer, nsIDOMWindow* aWindow,
This is probably also not required anymore.
::: toolkit/components/remote/nsRemoteService.h:25
(Diff revision 2)
> + // We will be a static singleton, so don't use the ordinary methods.
> + NS_DECL_ISUPPORTS
> + NS_DECL_NSIREMOTESERVICE
> + NS_DECL_NSIOBSERVER
> +
> + void SetDesktopStartupIDOrTimestamp(const nsACString& aDesktopStartupID,
The only call I can see is from nsRemoteService, so can be private.
::: toolkit/components/remote/nsRemoteService.h:27
(Diff revision 2)
> + NS_DECL_NSIREMOTESERVICE
> + NS_DECL_NSIOBSERVER
> +
> + void SetDesktopStartupIDOrTimestamp(const nsACString& aDesktopStartupID,
> + uint32_t aTimestamp);
> + const char* HandleCommandLine(const char* aBuffer, nsIDOMWindow* aWindow,
Please make a static method, this should avoid need to pass pointer to the nsRemoteService to the nsDBusRemoteService and nsGtkRemoteService.
::: toolkit/components/remote/nsRemoteService.h:30
(Diff revision 2)
> + RefPtr<nsIRemoteService> mDBusRemoteService;
> + RefPtr<nsIRemoteService> mGtkRemoteService;
You can use nsCOMPtr instead of RefPtr there.
::: toolkit/components/remote/nsRemoteService.cpp:37
(Diff revision 2)
> +NS_IMETHODIMP
> +nsRemoteService::Startup(const char* aAppName, const char* aProfileName)
> +{
> +#if defined(MOZ_ENABLE_DBUS)
> + nsresult rv;
> + mDBusRemoteService = static_cast<nsIRemoteService*>(new nsDBusRemoteService(this));
When mDBusRemoteService is nsCOMPtr the mDBusRemoteService = new nsDBusRemoteService();
should work there.
::: toolkit/components/remote/nsRemoteService.cpp:45
(Diff revision 2)
> + mDBusRemoteService = nullptr;
> + }
> +#endif
> +
> + if (GDK_IS_X11_DISPLAY(gdk_display_get_default())) {
> + mGtkRemoteService = static_cast<nsIRemoteService*>(new nsGTKRemoteService(this));
same as above
::: toolkit/components/remote/nsRemoteService.cpp:66
(Diff revision 2)
> +NS_IMETHODIMP
> +nsRemoteService::RegisterWindow(mozIDOMWindow* aWindow)
> +{
> +#if defined(MOZ_ENABLE_DBUS)
> + if (mDBusRemoteService) {
> + mDBusRemoteService->RegisterWindow(aWindow);
If calling nsDBusRemoteService::RegisterWindow actually do nothing consider removing this call completely.
::: toolkit/components/remote/nsXRemoteService.h:11
(Diff revision 2)
> * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>
> #ifndef NSXREMOTESERVICE_H
> #define NSXREMOTESERVICE_H
>
> +#include "nsRemoteService.h"
Is this a really required there?
::: toolkit/components/remote/nsXRemoteService.h:36
(Diff revision 2)
> -
> -
> protected:
> nsXRemoteService();
>
> - static bool HandleNewProperty(Window aWindowId,Display* aDisplay,
> + bool HandleNewProperty(Window aWindowId,Display* aDisplay,
Why is it no longer possible to be a static member function?
::: toolkit/components/remote/nsXRemoteService.h:44
(Diff revision 2)
>
> void XRemoteBaseStartup(const char *aAppName, const char *aProfileName);
>
> void HandleCommandsFor(Window aWindowId);
> - static nsXRemoteService *sRemoteImplementation;
> -private:
> +
> + virtual const char* HandleCommandLine(char* aBuffer, nsIDOMWindow* aWindow,
Is this still required since nsRemoteService should do the job now?
Attachment #8919682 -
Flags: review?(jhorak) → review-
Assignee | ||
Comment 11•7 years ago
|
||
mozreview-review |
Comment on attachment 8919682 [details]
Bug 1360566 - [Wayland] - Implement XRemote Server by D-Bus,
https://reviewboard.mozilla.org/r/190596/#review204838
::: toolkit/components/remote/nsDBusRemoteService.cpp:204
(Diff revision 2)
> + nsAutoCString objectName;
> + objectName = nsPrintfCString("/org/mozilla/%s/Remote", aAppName);
It can't due to type-cast from nsPrintfCString to nsAutoCString.
Assignee | ||
Comment 12•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8919682 [details]
Bug 1360566 - [Wayland] - Implement XRemote Server by D-Bus,
https://reviewboard.mozilla.org/r/190596/#review204442
> There should not be need for that anymore, or do I miss something?
Removed completelly.
Assignee | ||
Comment 13•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8919682 [details]
Bug 1360566 - [Wayland] - Implement XRemote Server by D-Bus,
https://reviewboard.mozilla.org/r/190596/#review204442
> Why is it no longer possible to be a static member function?
Made static again.
Assignee | ||
Comment 14•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8919682 [details]
Bug 1360566 - [Wayland] - Implement XRemote Server by D-Bus,
https://reviewboard.mozilla.org/r/190596/#review204442
> Is it worth to move that on the very beginning of ::Shutdown() in case that early return to be sure that the mActiveGTKRemoteService won't stay initialized to some already freed pointer?
removed mActiveGTKRemoteService.
Assignee | ||
Comment 15•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8919682 [details]
Bug 1360566 - [Wayland] - Implement XRemote Server by D-Bus,
https://reviewboard.mozilla.org/r/190596/#review204442
> Please do doublecheck the unrefing of the mConnection. In the ::Shutdown, you've mentioned it is not require to unref, but there it is done.
dbus_connection_unref() is called by RefPtr here so explicit call dbus_connection_unref() is redundant.
Comment hidden (mozreview-request) |
Comment 17•7 years ago
|
||
mozreview-review |
Comment on attachment 8919682 [details]
Bug 1360566 - [Wayland] - Implement XRemote Server by D-Bus,
https://reviewboard.mozilla.org/r/190596/#review205316
Looks good. Just check the nits, wait for the try run and I think we're done.
::: toolkit/components/remote/moz.build:17
(Diff revision 3)
> ]
>
> XPIDL_MODULE = 'toolkitremote'
>
> SOURCES += [
> 'nsXRemoteService.cpp',
According to https://searchfox.org/mozilla-central/source/toolkit/components/moz.build the 'remote' subdir is added to the targets only when MOZ_ENABLE_XREMOTE is set (ie --without-x is not used). Is this what we want also for the nsDbusRemoteService? Can't the nsDbusRemoteService work without x support?
Please file a new bug for this if you plan to support remote service when --without-x.
::: toolkit/components/remote/nsGTKRemoteService.h:41
(Diff revision 3)
> -
> - virtual void SetDesktopStartupIDOrTimestamp(const nsACString& aDesktopStartupID,
> - uint32_t aTimestamp) override;
> -
> nsInterfaceHashtable<nsPtrHashKey<GtkWidget>, nsIWeakReference> mWindows;
> - GtkWidget* mServerWindow;
> + GtkWidget* mServerWindow;
nit: indentation
::: toolkit/components/remote/nsGTKRemoteService.cpp:45
(Diff revision 3)
> mServerWindow = gtk_invisible_new();
> gtk_widget_realize(mServerWindow);
> HandleCommandsFor(mServerWindow, nullptr);
>
> for (auto iter = mWindows.Iter(); !iter.Done(); iter.Next()) {
> - HandleCommandsFor(iter.Key(), iter.UserData());
> + HandleCommandsFor(iter.Key(), iter.UserData());
nit: indent to two spaces
Attachment #8919682 -
Flags: review?(jhorak) → review+
Assignee | ||
Comment 18•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8919682 [details]
Bug 1360566 - [Wayland] - Implement XRemote Server by D-Bus,
https://reviewboard.mozilla.org/r/190596/#review205316
> According to https://searchfox.org/mozilla-central/source/toolkit/components/moz.build the 'remote' subdir is added to the targets only when MOZ_ENABLE_XREMOTE is set (ie --without-x is not used). Is this what we want also for the nsDbusRemoteService? Can't the nsDbusRemoteService work without x support?
>
> Please file a new bug for this if you plan to support remote service when --without-x.
IMHO that would be for Bug 1306613 - right now we don't support to build Gtk+ without X11.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 21•7 years ago
|
||
Pushed by stransky@redhat.com:
https://hg.mozilla.org/integration/autoland/rev/8a07d34a17b6
[Wayland] - Implement XRemote Server by D-Bus, r=jhorak
Comment 22•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Updated•6 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•