Bug 195167 - [CoordinatedGraphics] Unify DrawingArea classes
Summary: [CoordinatedGraphics] Unify DrawingArea classes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk, InRadar
Depends on: 195159
Blocks:
  Show dependency treegraph
 
Reported: 2019-02-28 08:44 PST by Carlos Garcia Campos
Modified: 2019-03-04 01:59 PST (History)
5 users (show)

See Also:


Attachments
Patch (140.38 KB, patch)
2019-02-28 08:46 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Patch (142.27 KB, patch)
2019-02-28 11:33 PST, Don Olmstead
no flags Details | Formatted Diff | Diff
Patch (141.91 KB, patch)
2019-02-28 12:35 PST, Don Olmstead
no flags Details | Formatted Diff | Diff
Patch (141.90 KB, patch)
2019-02-28 12:43 PST, Don Olmstead
no flags Details | Formatted Diff | Diff
Patch (141.92 KB, patch)
2019-02-28 12:58 PST, Don Olmstead
no flags Details | Formatted Diff | Diff
Patch (142.25 KB, patch)
2019-02-28 13:22 PST, Don Olmstead
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews112 for mac-highsierra (1.96 MB, application/zip)
2019-03-01 01:54 PST, Build Bot
no flags Details
Patch (141.10 KB, patch)
2019-03-01 04:12 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Patch (140.79 KB, patch)
2019-03-01 06:44 PST, Carlos Garcia Campos
zan: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2019-02-28 08:44:17 PST
In both UI and Web processes.
Comment 1 Carlos Garcia Campos 2019-02-28 08:46:01 PST
Created attachment 363223 [details]
Patch
Comment 2 Carlos Garcia Campos 2019-02-28 08:47:33 PST
It doesn't apply because it depends on bug #195159
Comment 3 Don Olmstead 2019-02-28 09:00:52 PST Comment hidden (obsolete)
Comment 4 Don Olmstead 2019-02-28 11:33:37 PST Comment hidden (obsolete)
Comment 5 Build Bot 2019-02-28 11:36:10 PST Comment hidden (obsolete)
Comment 6 Don Olmstead 2019-02-28 11:40:25 PST
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.
Comment 7 Don Olmstead 2019-02-28 11:50:49 PST
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.
Comment 8 Don Olmstead 2019-02-28 12:35:40 PST Comment hidden (obsolete)
Comment 9 Don Olmstead 2019-02-28 12:43:38 PST Comment hidden (obsolete)
Comment 10 Don Olmstead 2019-02-28 12:58:06 PST Comment hidden (obsolete)
Comment 11 Don Olmstead 2019-02-28 13:22:01 PST
Created attachment 363257 [details]
Patch
Comment 12 Don Olmstead 2019-02-28 14:46:46 PST
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.
Comment 13 Fujii Hironori 2019-02-28 19:06:47 PST
(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.
Comment 14 Carlos Garcia Campos 2019-02-28 22:41:37 PST
(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.
Comment 15 Carlos Garcia Campos 2019-02-28 22:44:28 PST
(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.
Comment 16 Carlos Garcia Campos 2019-02-28 22:47:49 PST
(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 17 Build Bot 2019-03-01 01:54:16 PST
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
Comment 18 Build Bot 2019-03-01 01:54:18 PST
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
Comment 19 Carlos Garcia Campos 2019-03-01 04:12:38 PST
Created attachment 363323 [details]
Patch
Comment 20 Fujii Hironori 2019-03-01 05:48:06 PST
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.
Comment 21 Carlos Garcia Campos 2019-03-01 06:44:21 PST
Created attachment 363326 [details]
Patch
Comment 22 Carlos Garcia Campos 2019-03-01 06:45:46 PST
(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 23 Zan Dobersek 2019-03-01 08:12:07 PST
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/?
Comment 24 Carlos Garcia Campos 2019-03-01 09:10:03 PST
(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.
Comment 25 Fujii Hironori 2019-03-03 19:32:35 PST
(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.
Comment 26 Fujii Hironori 2019-03-03 20:32:25 PST
(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.
Comment 27 Carlos Garcia Campos 2019-03-04 01:39:11 PST
(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.
Comment 28 Carlos Garcia Campos 2019-03-04 01:58:15 PST
Committed r242346: <https://trac.webkit.org/changeset/242346>
Comment 29 Radar WebKit Bug Importer 2019-03-04 01:59:34 PST
<rdar://problem/48556647>