Bug 191997

Summary: REGRESSION(r231043): [GTK] Undefined references to WebCore::LayerRepresentation::* with -DENABLE_OPENGL=OFF builds
Product: WebKit Reporter: Mart Raudsepp <leio>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: aperez, bugs-noreply, cadubentzen, commit-queue, mcatanzaro, pnormand, zan
Priority: P3 Keywords: Gtk
Version: WebKit Local Build   
Hardware: PC   
OS: Linux   
See Also: https://bugs.webkit.org/show_bug.cgi?id=191998
Attachments:
Description Flags
Patch none

Mart Raudsepp
Reported 2018-11-26 23:56:01 PST
webkit-gtk built without GL support leads to build/link failures of webkit-gtk itself, or any consumer (downstream reports had webkit-gtk consumers fail to link against 2.22.2, but I myself had trouble getting itself 2.22.4 built). /usr/lib/gcc/x86_64-pc-linux-gnu/7.3.0/../../../../lib64/libwebkit2gtk-4.0.so: undefined reference to `WebCore::LayerRepresentation::makePlatformLayerTypeless(WebCore::TextureMapperPlatformLayer*)' /usr/lib/gcc/x86_64-pc-linux-gnu/7.3.0/../../../../lib64/libwebkit2gtk-4.0.so: undefined reference to `WebCore::LayerRepresentation::releasePlatformLayer(void*)' /usr/lib/gcc/x86_64-pc-linux-gnu/7.3.0/../../../../lib64/libwebkit2gtk-4.0.so: undefined reference to `WebCore::LayerRepresentation::retainPlatformLayer(void*)' Michael Catanzaro helped track this down to probably being a regression since async scrolling was enabled: <mcatanzaro> Problem is ScrollingStateNode.h uses this function #if ENABLE(ASYNC_SCROLLING) || USE(COORDINATED_GRAPHICS) <mcatanzaro> But it's defined only #if USE(COORDINATED_GRAPHICS) <mcatanzaro> Guess: the non-GL build has been broken since we turned on ENABLE(ASYNC_SCROLLING) <mcatanzaro> And that was https://trac.webkit.org/changeset/231043/webkit <mcatanzaro> This is bug report territory now since the right fix is unclear. Maybe async scrolling needs to depend on GL.
Attachments
Patch (4.99 KB, patch)
2018-12-05 19:28 PST, Carlos Bentzen 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Michael Catanzaro
Comment 1 2018-11-27 02:03:05 PST
The fix for this is not obvious to me, but I guess the build guards are wrong in ScrollingStateNode.h: #if ENABLE(ASYNC_SCROLLING) || USE(COORDINATED_GRAPHICS) Expresses the intent for this file to work if USE(COORDINATED_GRAPHICS) is disabled, but it doesn't. Another option would be to add explicit dependencies at the CMake level if async scrolling really is supposed to depend on coordinated graphics, but they would have to be duplicated in both OptionsGTK.cmake and OptionsWPE.cmake because ENABLE_OPENGL is not a cross-platform option: WEBKIT_OPTION_DEPEND(ENABLE_ASYNC_SCROLLING ENABLE_OPENGL)
Michael Catanzaro
Comment 2 2018-11-27 02:11:06 PST
(We also might want to consider removing the ENABLE_OPENGL, option since clearly nobody has attempted to build 2.22 with it disabled before now. It exists only for embedded systems, and we never test building without it. But nowadays we expect embedded systems will mostly want to use WPE, and WPE requires OpenGL, so maybe it doesn't make sense anymore. Not sure.)
Carlos Bentzen
Comment 3 2018-12-05 19:28:17 PST
Created attachment 356698 [details] Patch
Carlos Bentzen
Comment 4 2018-12-05 19:30:15 PST
(In reply to Michael Catanzaro from comment #1) > Another option would be to add explicit dependencies at the CMake level if > async scrolling really is supposed to depend on coordinated graphics, but > they would have to be duplicated in both OptionsGTK.cmake and > OptionsWPE.cmake because ENABLE_OPENGL is not a cross-platform option: > > WEBKIT_OPTION_DEPEND(ENABLE_ASYNC_SCROLLING ENABLE_OPENGL) Did that in OptionsGTK.cmake but it is not need for WPE because 1) there's no ENABLE_OPENGL for WPE 2) ENABLE_ASYNC_SCROLLING is always on there explicitly
Adrian Perez
Comment 5 2018-12-06 03:41:27 PST
(In reply to Carlos Eduardo Ramalho from comment #4) > (In reply to Michael Catanzaro from comment #1) > > Another option would be to add explicit dependencies at the CMake level if > > async scrolling really is supposed to depend on coordinated graphics, but > > they would have to be duplicated in both OptionsGTK.cmake and > > OptionsWPE.cmake because ENABLE_OPENGL is not a cross-platform option: > > > > WEBKIT_OPTION_DEPEND(ENABLE_ASYNC_SCROLLING ENABLE_OPENGL) > > Did that in OptionsGTK.cmake but it is not need for WPE because > 1) there's no ENABLE_OPENGL for WPE > 2) ENABLE_ASYNC_SCROLLING is always on there explicitly JFTR, this seems like a reasonably good approach to me.
WebKit Commit Bot
Comment 6 2018-12-06 04:40:56 PST
Comment on attachment 356698 [details] Patch Clearing flags on attachment: 356698 Committed r238928: <https://trac.webkit.org/changeset/238928>
WebKit Commit Bot
Comment 7 2018-12-06 04:40:57 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.