Bug 144561

Created attachment 252296 [details] Patch to re-enable Quartz backend

Attachment 252296 [details] did not pass style-queue: Total errors found: 0 in 0 files If any of these errors are false positives, please file a bug against check-webkit-style.

changelog please

Comment on attachment 252296 [details] Patch to re-enable Quartz backend View in context: https://bugs.webkit.org/attachment.cgi?id=252296&action=review Thanks for the patch. Is this patch for master? If so, I think it needs to be rebased. Also, as Csaba mentioned all patches need ChangeLogs. > b/Source/cmake/OptionsGTK.cmake:181 > +if (ENABLE_QUARTZ_TARGET) > + add_definitions(-DWTF_PLATFORM_QUARTZ=1) > +endif () We expose variables to the build using SET_AND_EXPOSE_TO_BUILD now. > b/Source/cmake/FindGTK3.cmake:66 > + if (NOT("${GTK3_QUARTZ_VERSION}" VERSION_EQUAL "${GTK3_VERSION}")) > + set(ENABLE_QUARTZ_TARGET OFF) > + endif () Here you should give an error, because we want the build to produce errors instead of silently disabling things now. > b/Source/cmake/FindGTK3.cmake:73 > +if (NOT(ENABLE_X11_TARGET OR ENABLE_WAYLAND_TARGET OR ENABLE_QUARTZ_TARGET)) > message(FATAL_ERROR "At least one of the following windowing targets must " > - "be enabled and also supported by the GTK+ dependency: X11, Wayland") > + "be enabled and also supported by the GTK+ dependency: X11, Wayland, " > + "Quartz") > endif () I don't think the help message should mention Quartz until the port is fully functional.

Comment on attachment 252296 [details] Patch to re-enable Quartz backend View in context: https://bugs.webkit.org/attachment.cgi?id=252296&action=review Thanks for the patch. Is this patch for master? If so, I think it needs to be rebased. Also, as Csaba mentioned all patches need ChangeLogs. > b/Source/cmake/OptionsGTK.cmake:181 > +if (ENABLE_QUARTZ_TARGET) > + add_definitions(-DWTF_PLATFORM_QUARTZ=1) > +endif () We expose variables to the build using SET_AND_EXPOSE_TO_BUILD now. > b/Source/cmake/FindGTK3.cmake:66 > + if (NOT("${GTK3_QUARTZ_VERSION}" VERSION_EQUAL "${GTK3_VERSION}")) > + set(ENABLE_QUARTZ_TARGET OFF) > + endif () Here you should give an error, because we want the build to produce errors instead of silently disabling things now. > b/Source/cmake/FindGTK3.cmake:73 > +if (NOT(ENABLE_X11_TARGET OR ENABLE_WAYLAND_TARGET OR ENABLE_QUARTZ_TARGET)) > message(FATAL_ERROR "At least one of the following windowing targets must " > - "be enabled and also supported by the GTK+ dependency: X11, Wayland") > + "be enabled and also supported by the GTK+ dependency: X11, Wayland, " > + "Quartz") > endif () I don't think the help message should mention Quartz until the port is fully functional.

I think the option should be defined with WEBKIT_OPTION_DEFINE in PlatformGTK.cmake, off by default and private (invisible/unsupported). The option should have appropriate conflicts on ENABLE_WAYLAND_TARGET or ENABLE_X11_TARGET as needed. (In reply to comment #5) > Here you should give an error, because we want the build to produce errors > instead of silently disabling things now. Well, it should be an error indeed, but in PlatformGTK.cmake, not there. The find module shouldn't have anything to do with which features to enable: it should just find things. It should export a variable, say GTK_QUARTZ_FOUND, to indicate if it is available. Then in PlatformGTK.cmake, if ENABLE_QUARTZ_TARGET is set but GTK_QUARTZ_FOUND is not, the build should fail using message(FATAL_ERROR ...). But this patch is only copying the existing format of FindGTK3.cmake, which touches both ENABLE_X11_TARGET and ENABLE_WAYLAND_TARGET currently. I didn't realize it was doing that (since it is just a find module and I didn't anticipate it having side effects). Anyway, that won't work as expected anymore since this code runs after the values of those settings have already gone into the FEATURE_DEFINES list, so you would wind up with e.g. ENABLE_X11_TARGET turned off for most of the build, but on in IDL files (which use FEATURE_DEFINES). (I think ENABLE_QUARTZ_TARGET doesn't make it into FEATURE_DEFINES at all, since it's not defined with WEBKIT_OPTION_DEFINE.) I should file another bug for that.

I filed bug #144613.

> Is this patch for master? It is for 2.8.0. If I have some time later then I can try to make a patch for master, but as I mentioned on one of the other bugs it is fairly inconvenient for me to test it due to the uncommon platform; any help that would make it easier for me is appreciated. > We expose variables to the build using SET_AND_EXPOSE_TO_BUILD now. Doesn't exist in 2.8 AFACIT, so I'll leave it like this for the 2.8 patch. > I don't think the help message should mention Quartz until the port is fully functional. After the spate of patches that I filed on Sunday, the port should be fully functional; at least it works for me. So I'd prefer to keep it in. > > Here you should give an error, because we want the build to produce errors > > instead of silently disabling things now. > > Well, it should be an error indeed, but in PlatformGTK.cmake, not there. The find > module shouldn't have anything to do with which features to enable: it should just > find things. It should export a variable, say GTK_QUARTZ_FOUND, to indicate if it is > available. Then in PlatformGTK.cmake, if ENABLE_QUARTZ_TARGET is set but > GTK_QUARTZ_FOUND is not, the build should fail using message(FATAL_ERROR ...). > > But this patch is only copying the existing format of FindGTK3.cmake, which touches > both ENABLE_X11_TARGET and ENABLE_WAYLAND_TARGET currently. So, should I make this give an error now? Or leave it the way it is for consistency with the existing code of FindGTK3.cmake and let it be fixed by #144613.

Created attachment 252693 [details] Patch for 2.8.0 Here's an updated patch for 2.8.0 with changelog.

(In reply to comment #8) > So, should I make this give an error now? Or leave it the way it is for > consistency with the existing code of FindGTK3.cmake and let it be fixed by > #144613. Um... let's just leave it as-is, since it will be really easy for me to fix as part of bug #144613, but a chore for you to fix on OS X.

Created attachment 252820 [details] Patch

Here's a patch for master.

I'm not sure about adding Quartz target support to 2.8 branch. It's not actually a feature, but it's not a regression either since we never supported it in WebKit2. What do other people think?

(In reply to comment #13) > I'm not sure about adding Quartz target support to 2.8 branch. It's not > actually a feature, but it's not a regression either since we never > supported it in WebKit2. What do other people think? If it's WebKit2 support, support should go into trunk, I think.

I don't see value in backporting fixes for PRIVATE experimental features, since developers using experimental features should be using trunk. And I don't think we should mark the option PUBLIC unless we have an EWS bot at the very least, since it will be constantly broken otherwise. (It would be nice to set up a bot.) It'd be nice to get all of these fixed nicely in trunk, though, rather than let them sit in Bugzilla forever, which is their fate unless one of you picks it up. They're mostly self-contained, easy reviews. (See bug #126492.)

(In reply to comment #13) > I'm not sure about adding Quartz target support to 2.8 branch. It's not > actually a feature, but it's not a regression either since we never > supported it in WebKit2. What do other people think? You might notice that the patch contains only fixes to the CMake files. This is literally just adding build system code to enable the option. I didn't have to make any changes to the WebKit2 code to re-enable Quartz, or implement anything new for it. All the other bug fixes I've had to make to get it working, would also break the X11 backend on OSX, for example, or else they are breakages due to combinations of supported options that just don't get exercised together very often. The --enable-quartz-target switch was always there in the Autotools build system, so I'd consider this a regression from switching to CMake.

I think it only worked on WK1, though. Anyway, a small nit: QUARTZ should be listed above WAYLAND in OptionsGTK.cmake everywhere you added it, since those sections are otherwise alphabetical.

Um, in FindGTK3.cmake as well. Let me do this for you; I'm working off of this, so it's a waste of time to ask you to move a couple lines around.

Created attachment 252928 [details] [GTK] Re-enable Quartz backend on cmake build system

Upon further consideration, Q comes before X.

Created attachment 252929 [details] [GTK] Re-enable Quartz backend on cmake build system

(In reply to comment #18) > Um, in FindGTK3.cmake as well. Let me do this for you; I'm working off of > this, so it's a waste of time to ask you to move a couple lines around. Thanks, much appreciated.

Note: all of the issues I mentioned above are fixed by the patch in bug #144613, which depends on the patch in this bug.

(In reply to comment #23) > Note: all of the issues I mentioned above are fixed by the patch in bug > #144613, which depends on the patch in this bug. I'm re-uploading the patch for master so that it no longer depends on bug #144613. This is because I want to move that bug forward, and we still haven't agreed on whether or not to support OS X in WK2. Since so few and small patches are currently required, it seems like a harmless good idea right now, but we've already agreed to make accelerated compositing mandatory and have no plans to implement accelerated compositing for OS X, so it's sure to break in the future and require significant work to update. Note that nobody can possibly be relying on this now, except maybe you, since the GTK+ WK2 API has never supported OS X before. And note also that implementing accelerated compositing for OS X would require significant time and technical expertise, so being realistic I doubt it will happen. The argument for not merging the patch is that if we don't merge the patch, nobody will waste time porting their OS X application to WK2 (which is often a major effort!) only to find OS X support removed again down the line.

Created attachment 255262 [details] [GTK][CMake] Re-enable Quartz backend on cmake build system

Created attachment 255264 [details] [GTK][CMake] Re-enable Quartz backend on cmake build system

Comment on attachment 252693 [details] Patch for 2.8.0 (Goodness, am I having trouble with the patch statuses!)

(In reply to comment #24) > [...] we still > haven't agreed on whether or not to support OS X in WK2. I'd be very disappointed if you decided not to support it, for reasons detailed below. However, I definitely understand not wanting to make any guarantees about it. How about making it opt-in only? That is, commit the rest of the OSX patches but not this one. Or, commit this patch but explicitly print out a warning message when the Quartz option is activated, saying that it is not guaranteed to be supported. (As a side note, I'd like to clarify that this issue is about supporting the compilation of WebKitGTK on OSX _with the Quartz GDK backend_. I don't see how you can prevent people from building WebKitGTK on OSX with the X11 GDK backend, since that goes through the same code paths as building it on Linux/X11. And indeed, most of the bugs blocking #126492 apply to both the X11 and Quartz backends on OSX.) > Note that nobody can possibly be relying on this now, except maybe you, > since the GTK+ WK2 API has never supported OS X before. [...] The > argument for not merging the patch is that if we don't merge the patch, > nobody will waste time porting their OS X application to WK2 (which is often > a major effort!) only to find OS X support removed again down the line. Respectfully, I think you might be misunderstanding the use case. As far as I know there are no OSX-only GTK applications, either using WK1 or not. I think the concern is more about Linux application developers who in principle would like their application run on all platforms where GTK runs, but have already ported it to WK2, unaware that that would cut them off from the OSX platform. As an example, the case in point and the reason why I have been trying to build WK2 in the first place is Devhelp. There is no reason why Devhelp shouldn't be cross-platform, and indeed GTK may claim to be cross-platform but the developer experience on any platform that GTK supports will be pretty terrible without something like Devhelp. I would be perfectly happy if Devhelp stuck with WK1 as I think there's no reason why Devhelp needs its webviews to be multiprocess. But when WebKitGTK 2.6 was released, Devhelp (and many other applications) were ported to WK2 because, I surmise, the developers were spooked by WK1 being deprecated. And indeed, there was some encouragement from the WebKit team in that direction as well. [1] Devhelp, by extension, blocks Gnome Builder from being built on OSX, since Builder depends on a version of Devhelp from after the port to WK2. Builder is another application that I'd really like to see packaged for OSX. > Since so few and > small patches are currently required, it seems like a harmless good idea > right now, but we've already agreed to make accelerated compositing > mandatory and have no plans to implement accelerated compositing for OS X, > so it's sure to break in the future and require significant work to update. > [...] And note also that > implementing accelerated compositing for OS X would require significant time > and technical expertise, so being realistic I doubt it will happen. I wasn't aware of this but thank you for bringing it to my attention. That makes it all the more important to me that at least one stable version of a recent WebKitGTK should just build on OSX. I suppose that will keep us going for a few years until the applications have all moved on to a minimum WebKitGTK version that requires accelerated compositing. :-/ A few questions though; excuse my technical ignorance, but is accelerated compositing being done outside of GDK? Will it also not be supported on OSX with the X11 GDK backend? If done outside of GDK, will you have to write separate implementations for X11 and Wayland? [1] https://mail.gnome.org/archives/desktop-devel-list/2014-October/msg00048.html

Well it would be lovely to support OS X if possible. Even lovelier to have an EWS bot so that we can catch commits that break GTK+ on OS X, as otherwise it's doomed to be constantly broken. But the only potential blocker (we haven't agreed yet) is this: > A few questions though; excuse my technical ignorance, but is accelerated > compositing being done outside of GDK? Yes, it's implemented in WebKit. Our concern is best summarized by the comment [1]: "we are going to be removing the non-OpenGL version of the texture mapper." Judging by your patch in [1], that's going to break OS X. [1] https://bugs.webkit.org/show_bug.cgi?id=138332#c26 > Will it also not be supported on OSX with the X11 GDK backend? I am not sure, maybe Martin or Zan would know, but I suspect it would. Basically, you want to go the opposite route than you did with bug #138332, and compile with EGL and the GL texture mapper. I don't know how hard that would be. > If done outside of GDK, will you have to write > separate implementations for X11 and Wayland? Yes. There are already patches for the Wayland support (see bug #115803), but the patches are languishing in Bugzilla. (It's also broken with XWayland, which is really bad for Fedora 23.) P.S. The most important reason to update your app to WebKit2 is to get security updates.

(In reply to comment #29) > Well it would be lovely to support OS X if possible. Even lovelier to have > an EWS bot so that we can catch commits that break GTK+ on OS X, as > otherwise it's doomed to be constantly broken. But the only potential > blocker (we haven't agreed yet) is this: > > > A few questions though; excuse my technical ignorance, but is accelerated > > compositing being done outside of GDK? > > Yes, it's implemented in WebKit. Our concern is best summarized by the > comment [1]: "we are going to be removing the non-OpenGL version of the > texture mapper." Judging by your patch in [1], that's going to break OS X. > > [1] https://bugs.webkit.org/show_bug.cgi?id=138332#c26 > > > Will it also not be supported on OSX with the X11 GDK backend? > > I am not sure, maybe Martin or Zan would know, but I suspect it would. > Basically, you want to go the opposite route than you did with bug #138332, > and compile with EGL and the GL texture mapper. I don't know how hard that > would be. I didn't try it either, (I just disabled it because it was easier to get the build working) so I can't say for sure it will break on OSX. My problem is I don't know much about GL, so I am probably not the best person to try to compile it. I mean, it must be able to work at some level, since GL does exist on OSX and presumably this will work on the Mac port... So you're saying that the GL texture mapper is the prerequisite to accelerated compositing? I've sent out a request to the gtk-osx-devel mailing list to see if anyone with more GL knowledge than me could help. > > If done outside of GDK, will you have to write > > separate implementations for X11 and Wayland? > > Yes. There are already patches for the Wayland support (see bug #115803), > but the patches are languishing in Bugzilla. (It's also broken with > XWayland, which is really bad for Fedora 23.) > > P.S. The most important reason to update your app to WebKit2 is to get > security updates. That's also a reason why I'd prefer not to cut off access to WebKit2 for OSX. [1] https://mail.gnome.org/archives/gtk-osx-devel-list/2015-June/msg00004.html

(In reply to comment #30) > So you're saying that the GL texture mapper is the prerequisite to > accelerated compositing? Yes!

(In reply to comment #31) > (In reply to comment #30) > > So you're saying that the GL texture mapper is the prerequisite to > > accelerated compositing? > > Yes! OK, if that's the only obstacle, then we'll see if anyone responds to my call for help. In the meantime, do you agree with my idea that the other fixes blocking GTK/OSX/Quartz support could still be reviewed and merged?

Yes I do, but I'm not a reviewer so I can't help with that myself.

(In reply to comment #32) > In the meantime, do you agree with my idea that the other fixes blocking > GTK/OSX/Quartz support could still be reviewed and merged? The CMake patch (attachment #255264 [details]) is a good start, but it doesn't apply to the current tree cleanly.

(In reply to comment #34) > (In reply to comment #32) > > In the meantime, do you agree with my idea that the other fixes blocking > > GTK/OSX/Quartz support could still be reviewed and merged? > > The CMake patch (attachment #255264 [details]) is a good start, but it > doesn't apply to the current tree cleanly. It applies fine for me on r186333 (current master). I don't know how to reset EWS to make it reconsider the patch, short of uploading the same patch to Bugzilla again.

Created attachment 260215 [details] Patch

Comment on attachment 260215 [details] Patch So that's what I did. This is the same patch applied to current master.

Comment on attachment 252296 [details] Patch to re-enable Quartz backend Cleared Martin Robinson's review+ from obsolete attachment 252296 [details] so that this bug does not appear in http://webkit.org/pending-commit.

This patch looks good to me, any other feedback pending from Martin/Zan/Michael?

It would indeed be great to enable GL at some point. But meanwhile I think we can land this.

Comment on attachment 260215 [details] Patch (In reply to comment #39) > This patch looks good to me, any other feedback pending from > Martin/Zan/Michael? This patch is missing WEBKIT_OPTION_DEFINE(ENABLE_QUARTZ_TARGET ...); please be sure to add that so it shows up in the list of options.

Created attachment 264483 [details] Patch

Updated, thanks. As with #144560, I'll mark it cq? when I've built with this patch again. Arguably it shouldn't land until #144785 is fixed though.

Patch LGTM

Comment on attachment 264483 [details] Patch Clearing flags on attachment: 264483 Committed r192095: <http://trac.webkit.org/changeset/192095>

All reviewed patches have been landed. Closing bug.