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

Description Mart Raudsepp 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.
Comment 1 Michael Catanzaro 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)
Comment 2 Michael Catanzaro 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.)
Comment 3 Carlos Bentzen 2018-12-05 19:28:17 PST
Created attachment 356698 [details]
Patch
Comment 4 Carlos Bentzen 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
Comment 5 Adrian Perez 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.
Comment 6 WebKit Commit Bot 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>
Comment 7 WebKit Commit Bot 2018-12-06 04:40:57 PST
All reviewed patches have been landed.  Closing bug.