Closed Bug 184613 Opened 18 years ago Closed 10 years ago

get xremote working for mozilla-Qt

Categories

(Core Graveyard :: X-remote, enhancement)

x86
Linux
enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla2.0b4

People

(Reporter: timeless, Assigned: mjarvin)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
*** Bug 197045 has been marked as a duplicate of this bug. ***
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.
Attachment #448482 - Flags: review?(roc)
Attachment #448482 - Flags: review?(roc) → review?(karlt)
Assignee: timeless → mjarvin
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.
Attachment #448482 - Flags: review?(karlt) → review-
(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 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).
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.
Attachment #448482 - Attachment is obsolete: true
Attachment #462583 - Flags: review?
Attachment #462583 - Flags: review? → review?(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.
Attachment #462583 - Flags: review?(karlt) → review+
Sorry for layout change, made mistake when merging :(
Attachment #462583 - Attachment is obsolete: true
Attachment #462824 - Flags: review?(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.
Attachment #462824 - Flags: review?(karlt) → review+
Fixed indentation.
Attachment #462824 - Attachment is obsolete: true
Attachment #463191 - Flags: approval2.0?
Attachment #463191 - Flags: approval2.0? → approval2.0+
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/2cf6079d86dd
Status: NEW → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b4
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.