534644 - e10s: implement Qt ipc/chromium backend

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Doug Turner (:dougt)]

The current backend doesn't compile.  It may be just a few tweaks, or there might be something better/smarter on Qt:

In file included from /home/dougt/mobile/tinderbox/mozilla-central/ipc/chromium/src/base/message_loop.cc:23:
/home/dougt/mobile/tinderbox/mozilla-central/ipc/chromium/src/base/message_pump_glib.h:57: error: ISO C++ forbids declaration of 'GMainContext' with no type
/home/dougt/mobile/tinderbox/mozilla-central/ipc/chromium/src/base/message_pump_glib.h:57: error: expected ';' before '*' token
/home/dougt/mobile/tinderbox/mozilla-central/ipc/chromium/src/base/message_pump_glib.h:64: error: ISO C++ forbids declaration of 'GSource' with no type
/home/dougt/mobile/tinderbox/mozilla-central/ipc/chromium/src/base/message_pump_glib.h:64: error: expected ';' before '*' token
/home/dougt/mobile/tinderbox/mozilla-central/ipc/chromium/src/base/message_pump_glib.h:72: error: 'GPollFD' does not name a type
/home/dougt/mobile/tinderbox/mozilla-central/ipc/chromium/src/base/lazy_instance.h: In member function 'Type* base::LazyInstance<Type, Traits>::Pointer() [with Type = base::ThreadLocalPointer<MessageLoop>, Traits = base::DefaultLazyInstanceTraits<base::ThreadLocalPointer<MessageLoop> >]':
/home/dougt/mobile/tinderbox/mozilla-central/ipc/chromium/src/base/message_loop.cc:79:   instantiated from here
/home/dougt/mobile/tinderbox/mozilla-central/ipc/chromium/src/base/lazy_instance.h:93: warning: cast from 'signed char (*)[4]' to 'base::ThreadLocalPointer<MessageLoop>*' increases required alignment of target type
make[5]: *** [message_loop.o] Error 1
make[5]: Leaving directory `/home/dougt/mobile/tinderbox/mobilebase/xulrunner/ipc/chromium'

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

This is basically bug 528147.  We gain absolutely nothing by using chromium "event loop" code on the XPCOM main thread, and we've lost by it making Mozilla life worse in several ways.  We should fix bug 528147 rather than hack up ipc/chromium to support Qt.  Please ping me if I can be of assistance.

Comment 2

[Benjamin Smedberg]

Presumably this bug is about other things as well, such as a QT replacement for gtk_plug/gtk_socket.

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: ipc/chromium patch

I gave Roy bad advice based on my not understanding how part of this code works.  This patch takes care of the ipc/chromium stuff.

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #2)
> Presumably this bug is about other things as well, such as a QT replacement for
> gtk_plug/gtk_socket.

That stuff doesn't go near ipc/chromium.

Comment 5

[Roy Frostig :froystig]

Attachment: Patch: Qt+IPC build completes successfully

Here's a patch that gets us past a full IPC-happy build with --enable-default-toolkit=cairo-qt.  (NB: at time of writing, might still need --disable-crashreporter due to its glib dependencies).

Note this doesn't fix the bug, i.e. expect crashes at runtime, especially when plugins become involved.  Presumably, we'll need some things to work before coming back to IPC land, such as in comment #2 and bug 464966.

Comment 6

[Doug Turner (:dougt)]

probably a good idea to split out the plugin changes into a different bug.

Comment 7

[Roy Frostig :froystig]

Comment on attachment 418963 [details] [diff] [review]
Patch: Qt+IPC build completes successfully

Since there's work to be done elsewhere in the tree, would actually be nice to have this reviewed and landed.  Asking additional review from Josh because of a Makefile change in modules/plugin/*

Comment 8

[Josh Aas]

Comment on attachment 418963 [details] [diff] [review]
Patch: Qt+IPC build completes successfully

I think bsmedberg would be a better reviewer than me, re-targeting.

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 418963 [details] [diff] [review]
Patch: Qt+IPC build completes successfully

>diff --git a/ipc/chromium/src/chrome/common/chrome_paths.cc b/ipc/chromium/src/chrome/common/chrome_paths.cc
>--- a/ipc/chromium/src/chrome/common/chrome_paths.cc
>+++ b/ipc/chromium/src/chrome/common/chrome_paths.cc
>@@ -32,6 +32,7 @@
> }
> 
> bool PathProvider(int key, FilePath* result) {
>+#ifndef CHROMIUM_MOZILLA_BUILD
>   // Some keys are just aliases...
>   switch (key) {
>     case chrome::DIR_APP:
>@@ -212,6 +213,9 @@
> 
>   *result = cur;
>   return true;
>+#else
>+  return true;
>+#endif
> }
> 

Nit: place your |#endif| just after |*result = cur;|, drop the |#else|.

>diff --git a/ipc/ipdl/Makefile.in b/ipc/ipdl/Makefile.in
>--- a/ipc/ipdl/Makefile.in
>+++ b/ipc/ipdl/Makefile.in
>@@ -100,3 +100,15 @@
> 	  --outcpp-dir=.				\
> 	  $(IPDLDIRS:%=-I$(topsrcdir)/%)		\
> 	  $^
>+
>+
>+# We #include some things in the dom/plugins/ directory that rely on
>+# toolkit libraries.
>+#
>+# FIXME: below is only for Qt toolkit, but we should do the right
>+# thing for the others as well (e.g. see layout/generic/Makefile.in)
>+ifeq ($(MOZ_WIDGET_TOOLKIT),qt)
>+CXXFLAGS	+= $(MOZ_QT_CFLAGS)
>+CFLAGS		+= $(MOZ_QT_CFLAGS)
>+EXTRA_DSO_LDOPTS += $(MOZ_QT_LIBS)
>+endif

This is bsmedberg's territory, but I don't understand why we have to do this for Qt but not for Gtk.  Are you sure there's not a more central location where these options should be added?

r+ for the non-build stuff.

Comment 10

[Benjamin Smedberg]

Yeah, adding the specific MOZ_QT_CFLAGS/LIBS doesn't sound correct. We already have TK_CFLAGS/TK_LIBS variables, are those not populated with the QT flags correctly? We should use those.

Comment 11

[Oleg Romashin (:romaxa)]

Attachment: Fixed TK_FLAGS, removed #else.

Comment 12

[Oleg Romashin (:romaxa)]

Attachment: Qt Electrolisys patch

Comment 13

[Oleg Romashin (:romaxa)]

Attachment: Test-ipc works with this patch, updated to electrolisys branch trunk

Comment 14

[Oleg Romashin (:romaxa)]

Attachment: Mozilla-central version

version without TabChild.cpp, TabTypes.h

Comment 15

[Oleg Romashin (:romaxa)]

Try server: QT-IPC-534644
WinMo and linux - success... need to wait for other servers

Comment 16

[Oleg Romashin (:romaxa)]

Attachment: Launch QApplication with gArgv/gArgc

Qt does not store command-line parameters and we have to give global gArgv/gArgc... otherwise

Comment 17

[Oleg Romashin (:romaxa)]

Successful build on try server: 
https://build.mozilla.org/tryserver-builds/[e-mail]-QT-IPC-534644-3/

Comment 18

[Benjamin Smedberg]

Comment on attachment 421830 [details] [diff] [review]
Fixed TK_FLAGS, removed #else.

>diff -r 97d53a0cecc7 ipc/chromium/src/base/message_loop.cc

> using base::Time;
>@@ -107,17 +107,17 @@ MessageLoop::MessageLoop(Type type)
>   } else {
>     DCHECK(type_ == TYPE_UI);
>     pump_ = new base::MessagePumpForUI();
>   }
> #elif defined(OS_POSIX)
>   if (type_ == TYPE_UI) {
> #if defined(OS_MACOSX)
>     pump_ = base::MessagePumpMac::Create();
>-#elif defined(OS_LINUX)
>+#elif defined(OS_LINUX) && !defined(CHROMIUM_MOZILLA_BUILD)
>     pump_ = new base::MessagePumpForUI();
> #endif  // OS_LINUX

It seems like this change is incorrect. Don't we need a MessagePumpForUI here? I'm not an expert, so cjones should help understand this.

>diff -r 97d53a0cecc7 ipc/ipdl/Makefile.in

>+# We #include some things in the dom/plugins/ directory that rely on
>+# toolkit libraries.
>+#
>+CXXFLAGS	+= $(TK_CFLAGS)
>+EXTRA_DSO_LDOPTS += $(TK_LIBS)

Here and in dom/plugins/Makefile.in it should not be necessary to set EXTRA_DSO_LDOPTS here, since these never build shared libraries.

>diff -r 97d53a0cecc7 layout/build/Makefile.in

> ifdef MOZ_ENABLE_GTK2
>-EXTRA_DSO_LDOPTS += $(MOZ_GTK2_LIBS) \
>+EXTRA_DSO_LDOPTS += $(TK_LIBS) \
> 		    -lXrender \
> 		    $(NULL)
> endif
> 
> ifdef MOZ_ENABLE_QT
>-EXTRA_DSO_LDOPTS += $(MOZ_QT_LIBS) \
>-		    $(NULL)
>+EXTRA_DSO_LDOPTS += $(TK_LIBS)
> endif

Why are these still in ifdefs? Can't we just do this unconditionally?

>diff -r 97d53a0cecc7 modules/plugin/base/src/Makefile.in

> ifdef MOZ_ENABLE_GTK2
>-CXXFLAGS        += $(MOZ_GTK2_CFLAGS)
>-CFLAGS          += $(MOZ_GTK2_CFLAGS)
>+CXXFLAGS        += $(TK_CFLAGS)
>+CFLAGS          += $(TK_CFLAGS)
> EXTRA_DSO_LDOPTS += -lgtkxtbin $(XLDFLAGS) $(XT_LIBS) $(MOZ_GTK2_LIBS) $(XLIBS) $(XEXT_LIBS) $(XCOMPOSITE_LIBS)
> endif           #MOZ_ENABLE_GTK2
> 
>-ifdef MOZ_ENABLE_QT
>-CXXFLAGS	+= $(MOZ_QT_CFLAGS) $(XEXT_LIBS) $(XCOMPOSITE_LIBS)
>-CFLAGS		+= $(MOZ_QT_CFLAGS) $(XEXT_LIBS) $(XCOMPOSITE_LIBS)
>+ifeq ($(MOZ_WIDGET_TOOLKIT),qt)
>+CXXFLAGS        += $(TK_CFLAGS)
>+CFLAGS          += $(TK_CFLAGS)
>+EXTRA_DSO_LDOPTS += $(TK_LIBS) $(XEXT_LIBS) $(XCOMPOSITE_LIBS)
> endif

ditto

r=me with those changes, but need cjones to rule on the message loop and chromium path bits of this patch

Comment 19

[Oleg Romashin (:romaxa)]

Attachment: Updated mozilla-central patch, :bs comments

Comment 20

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 421830 [details] [diff] [review]
Fixed TK_FLAGS, removed #else.

(In reply to comment #18)
> (From update of attachment 421830 [details] [diff] [review])
> >diff -r 97d53a0cecc7 ipc/chromium/src/base/message_loop.cc
> 
> > using base::Time;
> >@@ -107,17 +107,17 @@ MessageLoop::MessageLoop(Type type)
> >   } else {
> >     DCHECK(type_ == TYPE_UI);
> >     pump_ = new base::MessagePumpForUI();
> >   }
> > #elif defined(OS_POSIX)
> >   if (type_ == TYPE_UI) {
> > #if defined(OS_MACOSX)
> >     pump_ = base::MessagePumpMac::Create();
> >-#elif defined(OS_LINUX)
> >+#elif defined(OS_LINUX) && !defined(CHROMIUM_MOZILLA_BUILD)
> >     pump_ = new base::MessagePumpForUI();
> > #endif  // OS_LINUX
> 
> It seems like this change is incorrect. Don't we need a MessagePumpForUI here?
> I'm not an expert, so cjones should help understand this.
> 

There's Mozilla special-case code earlier in that ctor that creates a special Gecko TYPE_UI MessagePump.  The Linux MessageLoopForUI() code is commented out so that we don't need to build message_pump_glib.cc, which is never used (and breaks Qt builds).

Comment 21

[Oleg Romashin (:romaxa)]

Attachment: Unreviewed part for Qt port m-c

Comment 22

[Oleg Romashin (:romaxa)]

Comment on attachment 421830 [details] [diff] [review]
Fixed TK_FLAGS, removed #else.

First reviewed part has been pushed in http://hg.mozilla.org/mozilla-central/rev/77f8b0f93f38

Comment 23

[Oleg Romashin (:romaxa)]

Attachment: Fix initial compilation on Qt

Rest of changes which are not reviewed yet.

Comment 24

[Benjamin Smedberg]

Comment on attachment 423950 [details] [diff] [review]
Fix initial compilation on Qt

Please fix the indentation in nsAppShell::ProcessNextNativeEvent (the returns are 3-space indented instead of 4). Otherwise looks good.

Comment 25

[Oleg Romashin (:romaxa)]

Pushed with fixed indentation in:
http://hg.mozilla.org/mozilla-central/rev/b7912eea7f1f

Need to check what else came from e10s branch.

Comment 26

[Benjamin Smedberg]

Note: I'm un-doing part of this change in bug 516515.

Comment 27

[Oleg Romashin (:romaxa)]

Attachment: Qt port compilation fix (caused by bug 516515)

Comment 28

[Benjamin Smedberg]

Comment on attachment 425167 [details] [diff] [review]
Qt  port compilation fix (caused by bug 516515)

The ifdef in MessageLoop::MessageLoop will cause things to build, but they won't work because you do actually need a Qt MessagePumpForUI implementation (at least for plugins). I suggest you --disable-ipc until that is implemented.

Comment 29

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

You can also use TYPE_MOZILLA_CHILD instead of TYPE_UI on Qt, for the time being.  (Quick warning: that's not as easy and localized of a change as it might sound.)

Comment 30

[Oleg Romashin (:romaxa)]

Attachment: Enable glib compilation with Qt port until message_pump_qt is not ready.

Comment 31

[Oleg Romashin (:romaxa)]

Attachment: Small patch, fixed MOZ_IPC + QT check for glib. only build fix part

Comment 32

[Oleg Romashin (:romaxa)]

Attachment: Fixed comment

Comment 33

[Doug Turner (:dougt)]

Comment on attachment 426177 [details] [diff] [review]
Fixed comment

with:

+#if defined(MOZ_WIDGET_GTK2)

Comment 34

[Oleg Romashin (:romaxa)]

Comment on attachment 426176 [details] [diff] [review]
Small patch, fixed MOZ_IPC + QT check for glib. only build fix part

This is obsolete, because we have already working QT message pump implementation in bug 544190

Comment 35

[Doug Turner (:dougt)]

oleg, marking wontfix based on your comment.

Comment 36

[Oleg Romashin (:romaxa)]

I don't this it is good idea to mark this bug as wontfix, until Qt port is compilable and runnable...

I was planning to keep this bug as some sort of tracking bug...

Comment 37

[Oleg Romashin (:romaxa)]

Ok, seems with patch from bug 545899 + patches from e10s mobile wiki page

no-more-tiers
serialize-gfxMatrix
bug_544623_shared_actor_tree
bug_542907
attachment.cgi?id=425581
attachment.cgi?id=426019

Make possible to run fennec with e10s

Comment 38

[Oleg Romashin (:romaxa)]

Ok, seems all critical patches landed into m-c, e10s repos...
Need some merge m-c -> e10s, and everything should be compilable and runnable.

Marking as fixed