Bug 195167 – [CoordinatedGraphics] Unify DrawingArea classes

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Carlos Garcia Campos]

In both UI and Web processes.

[Carlos Garcia Campos]

Created attachment 363223 [details]
Patch

[Carlos Garcia Campos]

It doesn't apply because it depends on bug #195159

[Don Olmstead]

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.

[Don Olmstead]

Created attachment 363243 [details]
Patch

Fix build

[EWS Watchlist]

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.

[Don Olmstead]

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.

[Don Olmstead]

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.

[Don Olmstead]

Created attachment 363251 [details]
Patch

Hopefully fix style and WPE build

[Don Olmstead]

Created attachment 363253 [details]
Patch

[Don Olmstead]

Created attachment 363254 [details]
Patch

[Don Olmstead]

Created attachment 363257 [details]
Patch

[Don Olmstead]

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.

[Fujii Hironori]

(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.

[Carlos Garcia Campos]

(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.

[Carlos Garcia Campos]

(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.

[Carlos Garcia Campos]

(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 :-)

[EWS Watchlist]

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

[EWS Watchlist]

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

[Carlos Garcia Campos]

Created attachment 363323 [details]
Patch

[Fujii Hironori]

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.

[Carlos Garcia Campos]

Created attachment 363326 [details]
Patch

[Carlos Garcia Campos]

(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.

[Zan Dobersek]

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/?

[Carlos Garcia Campos]

(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.

[Fujii Hironori]

(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.

[Fujii Hironori]

(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.

[Carlos Garcia Campos]

(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.

[Carlos Garcia Campos]

Committed r242346: <https://trac.webkit.org/changeset/242346>

[Radar WebKit Bug Importer]

<rdar://problem/48556647>