Bug 147256 – [CoordinatedGraphics] Rearrange uses of USE_COORDINATED_GRAPHICS_MULTIPROCESS

• Component: New Bugs
• Status: RESOLVED FIXED

[Gyuyoung Kim]

CoordinatedGraphics can work only when COORDINATED_GRAPHICS_MULTIPROCESS is enabled because the codes are already too coupled with whole WK2's coordinated graphics implementation. Even it is almost impossible to build WebKit EFL with disabling the COORDINATED_GRAPHICS_MULTIPROCESS guard. So I would recommend to remove this guard and let's only support COORDINATED_GRAPHICS_THREADED guard for EFL and GTK ports.

[Gyuyoung Kim]

Created attachment 257445 [details]
Patch

[Gwang Yoon Hwang]

Comment on attachment 257445 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=257445&action=review

> Source/WebKit2/WebProcess/WebPage/DrawingArea.cpp:42
>  #include "CoordinatedDrawingArea.h"

If we remove COORDINATED_GRAPHICS_MULTIPROCESS, we cannot separate MULTIPROCESS specific part of codes from THREADED mode.
For instance, the threaded mode do not uses CoordinatedDrawingArea.

[Gyuyoung Kim]

(In reply to comment #2)
> Comment on attachment 257445 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=257445&action=review
> 
> > Source/WebKit2/WebProcess/WebPage/DrawingArea.cpp:42
> >  #include "CoordinatedDrawingArea.h"
> 
> If we remove COORDINATED_GRAPHICS_MULTIPROCESS, we cannot separate
> MULTIPROCESS specific part of codes from THREADED mode.
> For instance, the threaded mode do not uses CoordinatedDrawingArea.

Can't we use COORDINATED_GRAPHICS_THREADED guard for the specific part ?

[Gyuyoung Kim]

(In reply to comment #3)
> (In reply to comment #2)
> > Comment on attachment 257445 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=257445&action=review
> > 
> > > Source/WebKit2/WebProcess/WebPage/DrawingArea.cpp:42
> > >  #include "CoordinatedDrawingArea.h"
> > 
> > If we remove COORDINATED_GRAPHICS_MULTIPROCESS, we cannot separate
> > MULTIPROCESS specific part of codes from THREADED mode.
> > For instance, the threaded mode do not uses CoordinatedDrawingArea.
> 
> Can't we use COORDINATED_GRAPHICS_THREADED guard for the specific part ?

Many codes are related with COORDINATED_GRAPHICS_MULTIPROCESS guard though, it didn't use it. So now we can't build WebKit, and even if we succeed to build WebKit disabled COORDINATED_GRAPHICS_MULTIPROCESS, I guess it doesn't work. That's why I suggest to use COORDINATED_GRAPHICS_MULTIPROCESS by default.

[Gwang Yoon Hwang]

(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > Comment on attachment 257445 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=257445&action=review
> > > 
> > > > Source/WebKit2/WebProcess/WebPage/DrawingArea.cpp:42
> > > >  #include "CoordinatedDrawingArea.h"
> > > 
> > > If we remove COORDINATED_GRAPHICS_MULTIPROCESS, we cannot separate
> > > MULTIPROCESS specific part of codes from THREADED mode.
> > > For instance, the threaded mode do not uses CoordinatedDrawingArea.
> > 
> > Can't we use COORDINATED_GRAPHICS_THREADED guard for the specific part ?
> 
> Many codes are related with COORDINATED_GRAPHICS_MULTIPROCESS guard though,
> it didn't use it. So now we can't build WebKit, and even if we succeed to
> build WebKit disabled COORDINATED_GRAPHICS_MULTIPROCESS, I guess it doesn't
> work. That's why I suggest to use COORDINATED_GRAPHICS_MULTIPROCESS by
> default.

COORDINATED_GRAPHICS_MULTIPROCESS is stands for to distinguish codes which are specific to process mode.
If we remove COORDINATED_GRAPHICS_MULTIPROCESS, we need to use !USE(COORDINATED_GRAPHICS_THREADED). However, I don't like it: I'll interpret this as a single thread mode instead of process mode.

[Gyuyoung Kim]

Created attachment 257524 [details]
Patch

[Gyuyoung Kim]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > (In reply to comment #2)
> > > > Comment on attachment 257445 [details]
> > > > Patch
> > > > 
> > > > View in context:
> > > > https://bugs.webkit.org/attachment.cgi?id=257445&action=review
> > > > 
> > > > > Source/WebKit2/WebProcess/WebPage/DrawingArea.cpp:42
> > > > >  #include "CoordinatedDrawingArea.h"
> > > > 
> > > > If we remove COORDINATED_GRAPHICS_MULTIPROCESS, we cannot separate
> > > > MULTIPROCESS specific part of codes from THREADED mode.
> > > > For instance, the threaded mode do not uses CoordinatedDrawingArea.
> > > 
> > > Can't we use COORDINATED_GRAPHICS_THREADED guard for the specific part ?
> > 
> > Many codes are related with COORDINATED_GRAPHICS_MULTIPROCESS guard though,
> > it didn't use it. So now we can't build WebKit, and even if we succeed to
> > build WebKit disabled COORDINATED_GRAPHICS_MULTIPROCESS, I guess it doesn't
> > work. That's why I suggest to use COORDINATED_GRAPHICS_MULTIPROCESS by
> > default.
> 
> COORDINATED_GRAPHICS_MULTIPROCESS is stands for to distinguish codes which
> are specific to process mode.
> If we remove COORDINATED_GRAPHICS_MULTIPROCESS, we need to use
> !USE(COORDINATED_GRAPHICS_THREADED). However, I don't like it: I'll
> interpret this as a single thread mode instead of process mode.

I see. However I still think that we need to re-arrange use of COORDINATED_GRAPHICS_MULTIPROCESS and COORDINATED_GRAPHICS_THREADED because those things have not been maintained so far. (Even I fail to build latest WebKit GTK with "--threaded-compositor".) This patch is probably first movement to maintain coordinated graphics from my side. 

Anyway I keep COORDINATED_GRAPHICS_MULTIPROCESS to distinguish between process model and threaded model. However I'd like to remove COORDINATED_GRAPHICS_MULTIPROCESS and COORDINATED_GRAPHICS_THREADED in future, then we will use one of those features using a setting flag.

[Gwang Yoon Hwang]

Comment on attachment 257524 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=257524&action=review

I'm agree to clean up multiprocess part of coordinated graphics, because it is too coupled with webview and webkit2 api, which makes complicated code structure.
However, I'm not sure to switch threaded / process model in runtime. Because it looks too complicated.
Maybe clean up UI Process part first would be preferable.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:125
>  #include "CoordinatedLayerTreeHostProxyMessages.h"

I'm not sure whether we should include CoordinaedLayerTreeHostProxy in the threaded mode.

> Source/WebKit2/WebProcess/WebPage/WebPage.h:1079
>      void findZoomableAreaForPoint(const WebCore::IntPoint&, const WebCore::IntSize& area);

This message is specific to MULTIPROCESS, do you want to implement in the threaded compositor, too?

[Gyuyoung Kim]

Created attachment 257553 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 257524 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=257524&action=review

>> Source/WebKit2/UIProcess/WebPageProxy.cpp:125
>>  #include "CoordinatedLayerTreeHostProxyMessages.h"
> 
> I'm not sure whether we should include CoordinaedLayerTreeHostProxy in the threaded mode.

Done.

>> Source/WebKit2/WebProcess/WebPage/WebPage.h:1079
>>      void findZoomableAreaForPoint(const WebCore::IntPoint&, const WebCore::IntSize& area);
> 
> This message is specific to MULTIPROCESS, do you want to implement in the threaded compositor, too?

Ok. I continue to use COORDINATED_GRAPHICS_MULTIPROCESS for findZoomableAreaForPoint().

[Gwang Yoon Hwang]

Comment on attachment 257553 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=257553&action=review

> Source/WebKit2/WebProcess/WebPage/DrawingArea.h:-115
> -#if USE(COORDINATED_GRAPHICS_MULTIPROCESS)

In threaded mode, we are not using CoordinatedLayerTreeHostMessage, so it should not be added.

> Source/WebKit2/WebProcess/WebPage/DrawingAreaImpl.h:77
> +#if USE(COORDINATED_GRAPHICS)

Ditto.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:-215
> -#if USE(COORDINATED_GRAPHICS_MULTIPROCESS)

Because in threaded mode we are not using CoordinatedLayerTreeHostMessages, 
It would be remained as _MULTIPROCESS

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:-494
> -#if USE(COORDINATED_GRAPHICS_MULTIPROCESS)

Ditto

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:-589
> -#if USE(COORDINATED_GRAPHICS_MULTIPROCESS)

Ditto

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:-3615
> -#if USE(COORDINATED_GRAPHICS_MULTIPROCESS)

Ditto.

[Gyuyoung Kim]

Created attachment 257741 [details]
Patch

[Gyuyoung Kim]

Created attachment 257746 [details]
Patch

[Gyuyoung Kim]

Yoon, for your information I succeed to build WebKit GTK with --threaded-compositor option.

[Gwang Yoon Hwang]

Comment on attachment 257746 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=257746&action=review

except some nitpik, I like this first step.

> Source/WebKit2/ChangeLog:9
> +        breaks when USE_COORDINATED_GRAPHICS_MULTIPROCESS is off.

I think this patch is a first step to enable COORDINATED_GRAPHICS_MULTIPROCESS and COORDINATED_GRAPHICS_THREADED in EFL and GTK port.

> Source/WebKit2/UIProcess/CoordinatedGraphics/WebPageProxyCoordinatedGraphics.cpp:53
> +#endif

I like this approach to mark coordinated graphics specific part in UI Process.

> Source/WebKit2/UIProcess/WebPageProxy.h:532
>  #endif

WebPageProxy::didRenderFrame is only used in multiprocess mode,
As you guard it explicitly at above modification: CoordinatedLayerTreeHostProxy.cpp.

> Source/WebKit2/UIProcess/WebPageProxy.h:615
>  #endif

WebPageProxy::commitPageTransitionViewport is also used in multiprocess mode.
I think we don't have to handle this in WebPage in threaded mode, too.

[Gyuyoung Kim]

Created attachment 257823 [details]
Patch

[Gyuyoung Kim]

(In reply to comment #15)
> Comment on attachment 257746 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=257746&action=review
> 
> except some nitpik, I like this first step.

Thanks !

> 
> > Source/WebKit2/ChangeLog:9
> > +        breaks when USE_COORDINATED_GRAPHICS_MULTIPROCESS is off.
> 
> I think this patch is a first step to enable
> COORDINATED_GRAPHICS_MULTIPROCESS and COORDINATED_GRAPHICS_THREADED in EFL
> and GTK port.
> 
> > Source/WebKit2/UIProcess/CoordinatedGraphics/WebPageProxyCoordinatedGraphics.cpp:53
> > +#endif
> 
> I like this approach to mark coordinated graphics specific part in UI
> Process.
> 
> > Source/WebKit2/UIProcess/WebPageProxy.h:532
> >  #endif
> 
> WebPageProxy::didRenderFrame is only used in multiprocess mode,
> As you guard it explicitly at above modification:
> CoordinatedLayerTreeHostProxy.cpp.
> 
> > Source/WebKit2/UIProcess/WebPageProxy.h:615
> >  #endif
> 
> WebPageProxy::commitPageTransitionViewport is also used in multiprocess mode.
> I think we don't have to handle this in WebPage in threaded mode, too.

Ok, all done.

[Gyuyoung Kim]

Created attachment 257837 [details]
Patch

[Gwang Yoon Hwang]

Comment on attachment 257837 [details]
Patch

Looks good to me!

[Gyuyoung Kim]

(In reply to comment #19)
> Comment on attachment 257837 [details]
> Patch
> 
> Looks good to me!

Ossy, could you take a look this ?

[Benjamin Poulain]

Comment on attachment 257837 [details]
Patch

rs=me.

I am not really familiar with that code but I don't see anything wrong with this.

[WebKit Commit Bot]

Comment on attachment 257837 [details]
Patch

Clearing flags on attachment: 257837

Committed r187795: <http://trac.webkit.org/changeset/187795>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.