In both UI and Web processes.
Created attachment 363223 [details] Patch
It doesn't apply because it depends on bug #195159
Comment on attachment 363223 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=363223&action=review Just saw that particular issue. I r+ed the dependency so just reupload afterwards and lets see what the bots say. > Source/WebKit/UIProcess/API/wpe/PageClientImpl.cpp:29 > +#include "DrawingAreaProxyDrawingAreaProxyCoordinatedGraphics.h" Looks like a find replace error here.
Created attachment 363243 [details] Patch Fix build
Attachment 363243 [details] did not pass style-queue: ERROR: Source/WebKit/WebProcess/WebPage/CoordinatedGraphics/DrawingAreaCoordinatedGraphics.h:67: Missing space before { [whitespace/braces] [5] ERROR: Source/WebKit/WebProcess/WebPage/CoordinatedGraphics/DrawingAreaCoordinatedGraphics.h:67: Missing space inside { }. [whitespace/braces] [5] ERROR: Source/WebKit/WebProcess/WebPage/CoordinatedGraphics/DrawingAreaCoordinatedGraphics.h:106: Missing space before { [whitespace/braces] [5] ERROR: Source/WebKit/WebProcess/WebPage/CoordinatedGraphics/DrawingAreaCoordinatedGraphics.h:109: Missing space before { [whitespace/braces] [5] ERROR: Source/WebKit/WebProcess/WebPage/CoordinatedGraphics/DrawingAreaCoordinatedGraphics.h:112: Missing space before { [whitespace/braces] [5] ERROR: Source/WebKit/WebProcess/WebPage/CoordinatedGraphics/DrawingAreaCoordinatedGraphics.h:116: Missing space before { [whitespace/braces] [5] ERROR: Source/WebKit/WebProcess/WebPage/CoordinatedGraphics/DrawingAreaCoordinatedGraphics.h:119: Missing space before { [whitespace/braces] [5] ERROR: Source/WebKit/WebProcess/WebPage/CoordinatedGraphics/DrawingAreaCoordinatedGraphics.h:123: Missing space before { [whitespace/braces] [5] ERROR: Source/WebKit/WebProcess/WebPage/CoordinatedGraphics/DrawingAreaCoordinatedGraphics.h:127: Missing space before { [whitespace/braces] [5] ERROR: Source/WebKit/WebProcess/WebPage/CoordinatedGraphics/DrawingAreaCoordinatedGraphics.h:131: Missing space before { [whitespace/braces] [5] ERROR: Source/WebKit/WebProcess/WebPage/CoordinatedGraphics/DrawingAreaCoordinatedGraphics.h:147: Missing space before { [whitespace/braces] [5] ERROR: Source/WebKit/WebProcess/WebPage/CoordinatedGraphics/DrawingAreaCoordinatedGraphics.h:149: Missing space before { [whitespace/braces] [5] ERROR: Source/WebKit/WebProcess/WebPage/CoordinatedGraphics/DrawingAreaCoordinatedGraphics.h:150: Missing space before { [whitespace/braces] [5] ERROR: Source/WebKit/UIProcess/CoordinatedGraphics/DrawingAreaProxyCoordinatedGraphics.cpp:412: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/WebProcess/WebPage/CoordinatedGraphics/DrawingAreaCoordinatedGraphics.cpp:38: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebKit/WebProcess/WebPage/CoordinatedGraphics/DrawingAreaCoordinatedGraphics.cpp:693: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] ERROR: Source/WebKit/UIProcess/CoordinatedGraphics/DrawingAreaProxyCoordinatedGraphics.h:30: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebKit/UIProcess/CoordinatedGraphics/DrawingAreaProxyCoordinatedGraphics.h:93: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/CoordinatedGraphics/DrawingAreaProxyCoordinatedGraphics.h:102: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 19 in 19 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 363243 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=363243&action=review This is building locally. I'll see what the bots have to say about it. One nit on this patch is that there's ports not turning on CoordinatedGraphics are taking in code that's marked as CoordinatedGraphics. Its a bit weird seeing USE(COORDINATED_GRAPHICS) inside there. Maybe a better name is required. Also as a followup it might be nice to have a platform specific cmake or SourcesCoordinatedGraphics.txt to keep the ports in sync with builds. > Source/WebKit/PlatformWin.cmake:-83 > - WebProcess/WebPage/DrawingAreaImpl.cpp This was still there in the original patch so CMake failed to create the project. > Source/WebKit/WebProcess/WebPage/DrawingArea.h:147 > -#if USE(COORDINATED_GRAPHICS) > - virtual void didChangeViewportAttributes(WebCore::ViewportAttributes&&) = 0; > -#endif > - > #if USE(COORDINATED_GRAPHICS) || USE(TEXTURE_MAPPER) > + virtual void didChangeViewportAttributes(WebCore::ViewportAttributes&&) = 0; > virtual void deviceOrPageScaleFactorChanged() = 0; > #endif I moved didChangeViewportAttributes because its in DrawingAreaCoordinatedGraphics and WinCairo errored since it was marked as override. It looks like these two methods could probably just be moved completely to DrawingAreaCoordinatedGraphics but I did not do so since I'm just making this patch compile.
wtf/glib/RunLoopSourcePriority in the #PLATFORM(WPE) portion does not have NonAcceleratedDrawingTimer defined so that's failing. Not sure what that number should be for that platform but that'll need to be in this patch.
Created attachment 363251 [details] Patch Hopefully fix style and WPE build
Created attachment 363253 [details] Patch
Created attachment 363254 [details] Patch
Created attachment 363257 [details] Patch
Ok everything is building now! I would probably still double check that Gentoo works with GTK since it doesn't have GL, but everything else should be reviewable. I asked aperezdc about the value for WPE and he said it shouldn't matter since you can't run WPE without AC. Feel free to change accordingly if that assumption is correct.
(In reply to Don Olmstead from comment #6) > Maybe a better name is required. Year. I always think so. USE(COORDINATED_GRAPHICS) was originally named USE(UI_SIDE_COMPOSITING). Then, renamed USE(COORDINATED_GRAPHICS_MULTIPROCESS). Then, it's been removed with Qt port and EFL port removal. 'Coordinated' originally meant being cooperative between two processes.
(In reply to Don Olmstead from comment #6) > Comment on attachment 363243 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=363243&action=review > > This is building locally. I'll see what the bots have to say about it. > > > One nit on this patch is that there's ports not turning on > CoordinatedGraphics are taking in code that's marked as CoordinatedGraphics. > Its a bit weird seeing USE(COORDINATED_GRAPHICS) inside there. Maybe a > better name is required. That's only the case of WinCairo, right? and the plan is to switch to coordinated graphics eventually, right? Fujii proposed to rename it once WinCairo switches, but I thought we could just name it coordinated graphics directly, even if the current situation is a bit weird for WinCairo. In any case, I don't mind to use a better name, any proposal? > Also as a followup it might be nice to have a platform specific cmake or > SourcesCoordinatedGraphics.txt to keep the ports in sync with builds. > > > Source/WebKit/PlatformWin.cmake:-83 > > - WebProcess/WebPage/DrawingAreaImpl.cpp > > This was still there in the original patch so CMake failed to create the > project. Forgot to remove that one, thanks! > > Source/WebKit/WebProcess/WebPage/DrawingArea.h:147 > > -#if USE(COORDINATED_GRAPHICS) > > - virtual void didChangeViewportAttributes(WebCore::ViewportAttributes&&) = 0; > > -#endif > > - > > #if USE(COORDINATED_GRAPHICS) || USE(TEXTURE_MAPPER) > > + virtual void didChangeViewportAttributes(WebCore::ViewportAttributes&&) = 0; > > virtual void deviceOrPageScaleFactorChanged() = 0; > > #endif > > I moved didChangeViewportAttributes because its in > DrawingAreaCoordinatedGraphics and WinCairo errored since it was marked as > override. > > It looks like these two methods could probably just be moved completely to > DrawingAreaCoordinatedGraphics but I did not do so since I'm just making > this patch compile. I see.
(In reply to Don Olmstead from comment #12) > Ok everything is building now! I would probably still double check that > Gentoo works with GTK since it doesn't have GL, but everything else should > be reviewable. I'll run a GTK build with OpenGL disabled to make sure it still builds. > I asked aperezdc about the value for WPE and he said it shouldn't matter > since you can't run WPE without AC. Feel free to change accordingly if that > assumption is correct. Right, I'm not sure if we should #ifdef the non-ac mode in WPE to make it explicit.
(In reply to Fujii Hironori from comment #13) > (In reply to Don Olmstead from comment #6) > > Maybe a better name is required. > > Year. I always think so. > USE(COORDINATED_GRAPHICS) was originally named USE(UI_SIDE_COMPOSITING). > Then, renamed USE(COORDINATED_GRAPHICS_MULTIPROCESS). > Then, it's been removed with Qt port and EFL port removal. > 'Coordinated' originally meant being cooperative between two processes. I agree, but that affects all current code under USE(COORDINATED_GRAPHICS), right? In this case, there's nothing specific to coordinated graphics in the DrawingArea implementation, it actually means the drawing area implementation of coordinated graphics based ports (even if it's not true for WinCairo yet). I'm open to use a better name, but Impl definitely isn't :-)
Comment on attachment 363257 [details] Patch Attachment 363257 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/11327915 New failing tests: storage/indexeddb/modern/blocked-open-db-requests-private.html
Created attachment 363315 [details] Archive of layout-test-results from ews112 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews112 Port: mac-highsierra Platform: Mac OS X 10.13.6
Created attachment 363323 [details] Patch
Comment on attachment 363323 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=363323&action=review > Source/WebKit/PlatformWin.cmake:117 > + "${WEBKIT_DIR}/UIProcess/CoordinatedGraphics" I hope WinCairo port will switch to CoordinatedGraphics to reduce the maintenance cost. However, I'm not satisfied with the current CoordinatedGraphics quality, for example in <https://webkit.org/blog-files/3d-transforms/morphing-cubes.html>. If this directory is add to the include path, WinCairo port can't have other LayerTreeHost.h to have different implementations temporarily until CoordinatedGraphics issue will be addressed. FWIW, I heard AppleWin port will switch to WK2. In that case, it will happen to resurrect DrawingAreaImpl again.
Created attachment 363326 [details] Patch
(In reply to Fujii Hironori from comment #20) > Comment on attachment 363323 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=363323&action=review > > > Source/WebKit/PlatformWin.cmake:117 > > + "${WEBKIT_DIR}/UIProcess/CoordinatedGraphics" > > I hope WinCairo port will switch to CoordinatedGraphics to reduce the > maintenance cost. > However, I'm not satisfied with the current CoordinatedGraphics quality, for > example in <https://webkit.org/blog-files/3d-transforms/morphing-cubes.html>. What's wrong there? > If this directory is add to the include path, WinCairo port can't have other > LayerTreeHost.h to have different implementations temporarily until > CoordinatedGraphics issue will be addressed. I see, let's find a better name for the common DrawingArea then. > FWIW, I heard AppleWin port will switch to WK2. In that case, it will happen > to resurrect DrawingAreaImpl again.
Comment on attachment 363326 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=363326&action=review > Source/WebKit/PlatformWin.cmake:81 > WebProcess/WebPage/AcceleratedSurface.cpp Can we also pull AcceleratedSurface files under WebProcess/WebPage/CoordinatedGraphics/?
(In reply to Zan Dobersek from comment #23) > Comment on attachment 363326 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=363326&action=review > > > Source/WebKit/PlatformWin.cmake:81 > > WebProcess/WebPage/AcceleratedSurface.cpp > > Can we also pull AcceleratedSurface files under > WebProcess/WebPage/CoordinatedGraphics/? Indeed, and move what we have in Shared to WebProcess too.
(In reply to Carlos Garcia Campos from comment #24) > Indeed, and move what we have in Shared to WebProcess too. How about the idea moving them into WebCore/platform/graphics/texmap/coordinated? They don't have WK2 dependency. We wouldn't need to check two directories.
(In reply to Carlos Garcia Campos from comment #22) > What's wrong there? Last week, I observed two issues in Morphing Power Cubes in both GTK port and WinCairo port with WIP patch (Bug 186364). One is sometimes the animation doesn'start (Bug 186364 Comment 8), another is "Toggle Shape" animation doesn't animate smoothly. However, I don't see both issues today. It seems your change of Bug 195208 addressed them.
(In reply to Fujii Hironori from comment #25) > (In reply to Carlos Garcia Campos from comment #24) > > Indeed, and move what we have in Shared to WebProcess too. > > How about the idea moving them into > WebCore/platform/graphics/texmap/coordinated? > They don't have WK2 dependency. We wouldn't need to check two directories. There's no dependency but it's only used by WebKit2, and only expected to be used by the WebProcess.
Committed r242346: <https://trac.webkit.org/changeset/242346>
<rdar://problem/48556647>