Closed Bug 917270 Opened 6 years ago Closed 6 years ago

Build widget for gtk2 and gtk3 in one pass

Categories

(Core :: Widget: Gtk, defect)

All
Linux
defect
Not set

Tracking

()

RESOLVED WONTFIX

People

(Reporter: stransky, Assigned: stransky)

References

Details

(Whiteboard: [leave open for remaining patches])

Attachments

(2 files, 4 obsolete files)

+++ This bug was initially created as a clone of Bug #624422 +++

For the gtk2 plugin support in a gtk3 build we need to build both versions simultaneously.
IMHO we may also rename widget/gtk2 to widget/gtk in this bug. 

widget/gtk2 contains gtk2 and gtk3 code so it makes sense to move it to widget/gtk and put gtk2 plugin code to widget/gtk/gtk2 subdir.
(In reply to Martin Stránský from comment #1)
> IMHO we may also rename widget/gtk2 to widget/gtk in this bug. 

Sounds good.
Attached patch move files from gtk2 to gtk (obsolete) — Splinter Review
Attachment #807145 - Flags: review?(karlt)
Attached patch build config update (obsolete) — Splinter Review
Build config update for gtk2->gtk directory rename
Attachment #807151 - Flags: review?(karlt)
Comment on attachment 807151 [details] [diff] [review]
build config update

Note: The gtk2_override.h file is used for building gtk2 builds in gtk3 environment, it undefines gtk3 build flags. We can't do that on gcc command line because it's reverted by preprocessor. 

I'll attach gtk2_override.h in other patches.
Attached patch widget/gtk/gtk2 subdir patch (obsolete) — Splinter Review
Attachment #807185 - Flags: review?
Comment on attachment 807185 [details] [diff] [review]
widget/gtk/gtk2 subdir patch

Creates gtk2 subdirs under widget directory. It covers widget/gtk, widget/shared. 

The shared module is linked to widget instead to xpwidgets on gtk, otherwise we have to build xpwidgets for both gtk2 and gtk3 too.
Attachment #807185 - Flags: review? → review?(karlt)
Comment on attachment 807145 [details] [diff] [review]
move files from gtk2 to gtk

Please change the commit message to say this is moving widget/gtk2 to widget/gtk, and include the necessary build changes (without MOZ_ENABLE_GTK2_PLUGIN changes) so that this revision will build and run tests.
Attachment #807145 - Flags: review?(karlt) → review-
Comment on attachment 807151 [details] [diff] [review]
build config update

Please move the gtk2_override.h changes to the subdir patch.
Attachment #807151 - Flags: review?(karlt) → review-
Comment on attachment 807185 [details] [diff] [review]
widget/gtk/gtk2 subdir patch

I'll pass this to a build config peer.

My suggestion to just build a standard GTK2 libxul in bug 624422 was assuming that such a build could use the existing buildmconfig, by doing two passes for example.

If this is going to be in one pass, given the GTK2 build is just for the plugin, can we just not build widget for the plugin library?  Does XPCOM give us enough separation to just not have the widget services/constructors?  That would be simpler than adding build info for things that are not used.
Attachment #807185 - Flags: review?(karlt) → review?(ted)
(In reply to Karl Tomlinson (:karlt) from comment #11)
> I'll pass this to a build config peer.
> 
> My suggestion to just build a standard GTK2 libxul in bug 624422 was
> assuming that such a build could use the existing buildmconfig, by doing two
> passes for example.
> 
> If this is going to be in one pass, given the GTK2 build is just for the
> plugin, can we just not build widget for the plugin library?  Does XPCOM
> give us enough separation to just not have the widget services/constructors?
> That would be simpler than adding build info for things that are not used.

My recent solution divides the code to two parts - the one which links gtk libraries (mark is as "gtk code" and the one without gtk parts ("the rest").

So the approach it do build "gtk code" twice (for gtk2 and gtk3) and link with "the rest" to two libraries, libxul.so (for gtk3) and libxul_gtk2.so (for gtk2).

Pros is the the "gtk code" is relatively small (widget/gtk, widget/shared, dom/plugins) so the build time is almost the same as a normal build. Cons is the you need extra directories for the gtk2 parts to allow them to build simultaneously.

I can try to remove the widget module from libxul_gtk2.so library but it may take more time. I'd like to do such optimization later, when the whole solution works.
Gtk2 rename in one patch without gtk2_override.h
Try: https://tbpl.mozilla.org/?tree=Try&rev=62594d75c865
Attachment #807145 - Attachment is obsolete: true
Attachment #807151 - Attachment is obsolete: true
Attachment #807699 - Flags: review?(karlt)
Attachment #807699 - Flags: review?(karlt) → review+
Keywords: checkin-needed
Attachment #807699 - Flags: checkin?
Comment on attachment 807699 [details] [diff] [review]
gtk2 rename patch

Please land this patch after Bug 914607.
Attachment #807699 - Flags: checkin? → checkin+
https://hg.mozilla.org/mozilla-central/rev/4aa7d035c1af
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla27
More patches are needed, sorry.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [leave open for remaining patches]
Target Milestone: mozilla27 → ---
(In reply to Karl Tomlinson (:karlt) from comment #11)
> My suggestion to just build a standard GTK2 libxul in bug 624422 was
> assuming that such a build could use the existing buildmconfig, by doing two
> passes for example.
> 
> If this is going to be in one pass, given the GTK2 build is just for the
> plugin, can we just not build widget for the plugin library?  Does XPCOM
> give us enough separation to just not have the widget services/constructors?
> That would be simpler than adding build info for things that are not used.

See my comment at https://bugzilla.mozilla.org/show_bug.cgi?id=624422#c15 - it's tricky to do such separation and even if we do so, the result would be difficult to maintain. It's not just widget/toolkit directory, the widget specific code is nested to other parts.

More specifically, those directories use gtk code and needs to be build in gtk2 and gtk3 variants:

dom/plugins/base
dom/plugins/ipc
gfx/gl
gfx/thebes
toolkit/components/remote
widget/gtk
widget/shared
Comment 11 was not about removing gtk use from every part of the code, but questioning whether some parts of the code were not necessary at all and so wouldn't need to be included at all in the "libxul_gtk2" for the plugin.

I can imagine that gfx/thebes and gfx/gl would be hard to drop, and the plugin will need dom/plugins, but I would expect toolkit/components/remote to be easy to remove.
I don't know about widget, so will leave this to your judgement.
I've passed review to the build peers.

Ted, I picked you because you commented in bug 624422, but feel free to redirect if you like.
Comment on attachment 807185 [details] [diff] [review]
widget/gtk/gtk2 subdir patch

Sorry, I'm not finding the time to review these patches. I'll punt to glandium who is probably more suited anyway.
Attachment #807185 - Flags: review?(ted) → review?(mh+mozilla)
(In reply to Karl Tomlinson (:karlt) from comment #19)
> I can imagine that gfx/thebes and gfx/gl would be hard to drop, and the
> plugin will need dom/plugins, but I would expect toolkit/components/remote
> to be easy to remove.

toolkit/components/remote is used by /toolkit/xre/nsAppRunner.cpp, which is executed by plugin-container...so removing remote leads to two-pass build of /toolkit/xre/ or nsAppRunner.cpp at least...or to nsAppRunnerGtk2.cpp file.
Summary: Build widget/gtk for gtk2 and gtk3 in one pass → Build widget for gtk2 and gtk3 in one pass
Attachment #807185 - Attachment description: gtk/gtk2 subdir patch → widget/gtk/gtk2 subdir patch
Attached patch patch to widget/shared (obsolete) — Splinter Review
Attachment #819736 - Flags: feedback?
Attachment #819736 - Flags: feedback? → feedback?(karlt)
Comment on attachment 819736 [details] [diff] [review]
patch to widget/shared

Ahh, it's already included in the widget patch.
Attachment #819736 - Attachment is obsolete: true
Attachment #819736 - Flags: feedback?(karlt)
(In reply to Martin Stránský from comment #21)
> (In reply to Karl Tomlinson (:karlt) from comment #19)
> > I can imagine that gfx/thebes and gfx/gl would be hard to drop, and the
> > plugin will need dom/plugins, but I would expect toolkit/components/remote
> > to be easy to remove.
> 
> toolkit/components/remote is used by /toolkit/xre/nsAppRunner.cpp, which is
> executed by plugin-container...so removing remote leads to two-pass build of
> /toolkit/xre/ or nsAppRunner.cpp at least...or to nsAppRunnerGtk2.cpp file.

Isn't the only interface nsIRemoteService.idl?
nsAppRunner looks like it tries to handle failure of do_GetService("@mozilla.org/toolkit/remote-service;1").
An updated one, with config changes.
Attachment #807185 - Attachment is obsolete: true
Attachment #807185 - Flags: review?(mh+mozilla)
Attachment #820177 - Flags: review?(mh+mozilla)
Comment on attachment 820177 [details] [diff] [review]
patch to widget, v.2

Review of attachment 820177 [details] [diff] [review]:
-----------------------------------------------------------------

Same reason as bug 929408.
Attachment #820177 - Flags: review?(mh+mozilla) → review-
Closing as per bug 929408. The one-pass build seems to be too difficult to maintain.
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Resolution: FIXED → WONTFIX
You need to log in before you can comment on or make changes to this bug.