184613 - get xremote working for mozilla-Qt

Status: RESOLVED FIXED

(Core Graveyard :: X-remote, enhancement)

Description

[timeless]

WARNING: couldn't get widget helper service, file
/mnt/hda3/temp/mozilla/xpfe/components/xremote/src/XRemoteService.cpp, line 370
JavaScript error:
 line 0: uncaught exception: [Exception... "Component returned failure code:
0x80004005 (NS_ERROR_FAILURE) [nsIXRemoteService.addBrowserInstance]"  nsresult:
"0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame ::
chrome://navigator/content/navigator.js :: Startup :: line 544"  data: no]

There's also bug 81332 Implement chrome based xremote support.
but I think I'll probably implement xremote for qt first instead.

Comment 1

[Christian :Biesinger (don't email me, ping me on IRC)]

*** Bug 197045 has been marked as a duplicate of this bug. ***

Comment 2

[Miika Jarvinen]

Attachment: Patch which adds xremote support for qt build

Attached patch which enables XRemote support for Qt build. I added base class for Qt/Gtk remote handling, so that XAtom handling would be in one place. 

On Qt we are not able to implement RegisterWindow interface, because invidual QWidgets XEvents can be handled only in QWidget::x11Event.

Patch is for mozilla-central revision 633756e2a580.

Comment 3

[Karl Tomlinson (:karlt)]

Comment on attachment 448482 [details] [diff] [review]
Patch which adds xremote support for qt build

Can some of the header includes in nsGTKRemoteService.cpp be removed now?
Similarly, are all the includes in nsXRemoteService.cpp necessary?

Checking for new properties is probably the only functional change necessary.
Most of my comments are style issues:

>   mServerWindow = gtk_invisible_new();
>   gtk_widget_realize(mServerWindow);
>-  HandleCommandsFor(mServerWindow, nsnull);
> 
>   if (!mWindows.IsInitialized())
>     mWindows.Init();
> 
>   mWindows.EnumerateRead(StartupHandler, this);
> 
>-  nsCOMPtr<nsIObserverService> obs
>-    (do_GetService("@mozilla.org/observer-service;1"));
>-  if (obs) {
>-    obs->AddObserver(this, "xpcom-shutdown", PR_FALSE);
>-    obs->AddObserver(this, "quit-application", PR_FALSE);
>-  }
>+
>+  HandleCommandsFor(mServerWindow, nsnull);

I can't see any reason for changing the order of
HandleCommandsFor(mServerWindow, nsnull).
I preferred the old order that kept the mServerWindow operations together.

>-static void
>-SetDesktopStartupIDOrTimestamp(const nsACString& aDesktopStartupID,
>+ void
>+         nsGTKRemoteService:: SetDesktopStartupIDOrTimestamp(const nsACString& aDesktopStartupID,

Touch up indentation and spaces.

>+#ifdef MOZ_WIDGET_GTK2
>+    Atom changedAtom = gdk_x11_atom_to_xatom(pevent->atom);
>+#else
>+    Atom changedAtom = pevent->atom;
>+#endif

Just assume that MOZ_WIDGET_GTK2 is defined.  (We don't support GTK+ 1.2
anymore, so no need for the conditional compilation.)

>-  if (pevent->state == GDK_PROPERTY_NEW_VALUE &&

I think the check for new properties should remain.

>+    return nsXRemoteService::HandlePropertyChange(GDK_WINDOW_XWINDOW(pevent->window),
>+                                GDK_DISPLAY(),
>+                                pevent->time,changedAtom,aThis);

The trailing lines can be indented to inside the opening "(", and Mozilla
style is spaces after ",".

>-  return FALSE;
>+  return FALSE;  
> }

Additional trailing whitespace.

>+class nsGTKRemoteService;
> 
>-class nsGTKRemoteService : public nsIRemoteService,
>-                           public nsIObserver
>+class nsGTKRemoteService : public nsXRemoteService

I assume the forward declaration is not necessary.

>+  Display *XDisplay();

We now have mozilla::DefaultXDisplay available:
http://hg.mozilla.org/mozilla-central/annotate/421492990143/gfx/src/X11Util.h#l64

>+
>+  void SetDesktopStartupIDOrTimestamp(const nsACString& aDesktopStartupID,
>+                                      PRUint32 aTimestamp);

Style is usually to include the virtual keyword even in derived classes to
make things clearer.

>+nsXRemoteService::Observe(nsISupports* aSubject,
>+                            const char *aTopic,
>+                            const PRUnichar *aData)

Touch up indentation.

>+    nsXRemoteService();

This can be protected, I think.

>+    NS_SCRIPTABLE NS_IMETHOD Startup(const char *aAppName, const char *aProfileName);

This also, I assume is only intended for use by derived classes, in which case
it can be protected and doesn't need NS_SCRIPTABLE or NS_IMETHOD.

I think it would be clearer if this were a non-virtual method with a different
name.  e.g BaseStartup or XRemoteStartup or XRemoteBaseStartup.

>+    static PRBool HandlePropertyChange(Window aWindowId,Display* aDisplay,
>+                                       Time aEventTime, Atom aChangedAtom,
>+                                       nsIWeakReference* aDomWindow);

Does this trigger any "is hidden by" gcc warnings for the derived-class method
of the same name?
I think it should probably be called something like HandleNewProperty anyway.

Need a space after "aWindowId,".

>+    static nsXRemoteService *sRemoteImplementation;

>+    static PRBool
>+    FindExtensionParameterInCommand(const char* aParameterName,
>+                                    const nsACString& aCommand,
>+                                    char aSeparator,
>+                                    nsACString* aValue);

Usually we use (file-scope) static-linkage functions if access to |this| is
not needed.  It saves unnecessarily adding to the header.

>+PRBool nsXRemoteService::HandlePropertyChange(XID aWindowId, Display* aDisplay,
>+                                              Time aEventTime,Atom aChangedAtom,
>+                                              nsIWeakReference* aDomWindow
>+                                              )

Return type on its own line, closing ")" at the end of previous line,
space after ",".

>+    // put the property onto the window as the response
>+    if (response)
>+    XChangeProperty (aDisplay, aWindowId,

>+nsXRemoteService::HandleCommand(char* aCommand, nsIDOMWindow* aWindow,
>+                                  PRUint32 aTimestamp)

Indentation.

>+nsXRemoteService::HandleCommandLine(char* aBuffer, nsIDOMWindow* aWindow,
>+                                    PRUint32 aTimestamp
>+                                    )

closing ")" at the end of previous line.

>+PRBool nsXRemoteService::FindExtensionParameterInCommand(const char* aParameterName,
>+                                const nsACString& aCommand,
>+                                char aSeparator,
>+                                nsACString* aValue)

Return type on own line, indentation.

>+#include <QWidget>

I don't think this needs to be in the header file.

>+  mServerWindow = new MozQRemoteEventHandlerWidget(*this);
>+  if (!mServerWindow) {
>+    return NS_ERROR_OUT_OF_MEMORY;
>+  }

|new| won't return null.

>+  void propertyNotifyEvent(XEvent *evt);

Mozilla style is that method names start with capital letters.

Comment 4

[Karl Tomlinson (:karlt)]

(In reply to comment #2)
> On Qt we are not able to implement RegisterWindow interface, because invidual
> QWidgets XEvents can be handled only in QWidget::x11Event.

If necessary, one option might be to open a new Display connection to listen for events and hook that up to the event loop.

That's probably a bit of work though and I don't know how important RegisterWindow is.  Don't try to do that in this patch, anyway.

Comment 5

[Karl Tomlinson (:karlt)]

Comment on attachment 448482 [details] [diff] [review]
Patch which adds xremote support for qt build

>+    // put the property onto the window as the response
>+    if (response)
>+    XChangeProperty (aDisplay, aWindowId,
>+                     sMozResponseAtom, XA_STRING,
>+                     8, PropModeReplace, (const unsigned char *)response, strlen (response));

No need for the null check here (and I'm guessing that there should always be some sort of protocol reply, if at all possible).

Comment 6

[Miika Jarvinen]

Attachment: Patch which adds xremote support for qt build

Updated patch according to review. 

Check for GDK_PROPERTY_NEW_VALUE  was there also in last patch, I just removed && from end of that line.

Comment 7

[Karl Tomlinson (:karlt)]

Comment on attachment 462583 [details] [diff] [review]
Patch which adds xremote support for qt build

> Check for GDK_PROPERTY_NEW_VALUE  was there also in last patch, I just removed
> && from end of that line.

Oh, sorry I missed that.

>--- a/layout/Makefile.in	Tue Aug 03 14:18:53 2010 -0400
>+++ b/layout/Makefile.in	Tue Aug 03 16:31:15 2010 -0700
>@@ -82,13 +82,13 @@
>   xul/test \
>   xul/base/test \
>   $(NULL)
> 
> TOOL_DIRS      += tools/reftest reftests/fonts reftests/fonts/mplus
> DIRS           += tools/pageloader
> 
> ifdef MOZ_DEBUG
>-DIRS            += tools/layout-debug
>+#DIRS            += tools/layout-debug

Remove this change.

>+nsGTKRemoteService::HandleNewProperty(GtkWidget *aWidget,
>+                                         GdkEventProperty *pevent,
>+                                         nsIWeakReference *aThis)

>-nsGTKRemoteService::HandlePropertyChange(GtkWidget *aWidget,
>-                                         GdkEventProperty *pevent,
>-                                         nsIWeakReference* aThis)
>-{

Leave this as nsGTKRemoteService::HandlePropertyChange,
but change nsXRemoteService::HandlePropertyChange
to nsXRemoteService::HandleNewProperty, please.


>+  virtual void SetDesktopStartupIDOrTimestamp(const nsACString& aDesktopStartupID,
>+                                      PRUint32 aTimestamp);

>+FindExtensionParameterInCommand(const char* aParameterName,
>+                                                  const nsACString& aCommand,
>+                                                  char aSeparator,
>+                                                  nsACString* aValue)

Indentation.

>+nsXRemoteService::HandleCommandLine(char* aBuffer, nsIDOMWindow* aWindow,
>+                                    PRUint32 aTimestamp
>+                                    )

Move closing ")" to end of previous line.

Comment 8

[Miika Jarvinen]

Attachment: Patch which adds xremote support for qt build

Sorry for layout change, made mistake when merging :(

Comment 9

[Karl Tomlinson (:karlt)]

Comment on attachment 462824 [details] [diff] [review]
Patch which adds xremote support for qt build

>+  HandleNewProperty(aEvt->xproperty.window,
>+                       QX11Info::display(),
>+                       aEvt->xproperty.time,
>+                       aEvt->xproperty.atom,
>+                       0);
>+}

Just touch up indentation here.
No need for further review, though currently you'll need to request approval for landing.

Comment 10

[Miika Jarvinen]

Attachment: Patch which adds xremote support for qt build

Fixed indentation.

Comment 11

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/2cf6079d86dd