Bug 147256 - [CoordinatedGraphics] Rearrange uses of USE_COORDINATED_GRAPHICS_MULTIPROCESS
Summary: [CoordinatedGraphics] Rearrange uses of USE_COORDINATED_GRAPHICS_MULTIPROCESS
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Gyuyoung Kim
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-07-24 00:54 PDT by Gyuyoung Kim
Modified: 2015-08-03 18:17 PDT (History)
3 users (show)

See Also:


Attachments
Patch (18.80 KB, patch)
2015-07-24 01:01 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (22.22 KB, patch)
2015-07-25 18:18 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (21.77 KB, patch)
2015-07-27 01:40 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (19.84 KB, patch)
2015-07-29 01:09 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (19.69 KB, patch)
2015-07-29 06:22 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (21.67 KB, patch)
2015-07-30 01:22 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (21.06 KB, patch)
2015-07-30 09:36 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gyuyoung Kim 2015-07-24 00:54:50 PDT
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.
Comment 1 Gyuyoung Kim 2015-07-24 01:01:09 PDT
Created attachment 257445 [details]
Patch
Comment 2 Gwang Yoon Hwang 2015-07-24 01:18:28 PDT
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.
Comment 3 Gyuyoung Kim 2015-07-24 01:40:17 PDT
(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 ?
Comment 4 Gyuyoung Kim 2015-07-24 01:49:01 PDT
(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.
Comment 5 Gwang Yoon Hwang 2015-07-24 05:09:24 PDT
(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.
Comment 6 Gyuyoung Kim 2015-07-25 18:18:59 PDT
Created attachment 257524 [details]
Patch
Comment 7 Gyuyoung Kim 2015-07-25 18:29:04 PDT
(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.
Comment 8 Gwang Yoon Hwang 2015-07-26 23:37:10 PDT
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?
Comment 9 Gyuyoung Kim 2015-07-27 01:40:33 PDT
Created attachment 257553 [details]
Patch
Comment 10 Gyuyoung Kim 2015-07-27 01:41:47 PDT
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().
Comment 11 Gwang Yoon Hwang 2015-07-27 06:42:10 PDT
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.
Comment 12 Gyuyoung Kim 2015-07-29 01:09:25 PDT
Created attachment 257741 [details]
Patch
Comment 13 Gyuyoung Kim 2015-07-29 06:22:05 PDT
Created attachment 257746 [details]
Patch
Comment 14 Gyuyoung Kim 2015-07-29 06:24:08 PDT
Yoon, for your information I succeed to build WebKit GTK with --threaded-compositor option.
Comment 15 Gwang Yoon Hwang 2015-07-29 23:33:36 PDT
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.
Comment 16 Gyuyoung Kim 2015-07-30 01:22:44 PDT
Created attachment 257823 [details]
Patch
Comment 17 Gyuyoung Kim 2015-07-30 01:23:47 PDT
(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.
Comment 18 Gyuyoung Kim 2015-07-30 09:36:21 PDT
Created attachment 257837 [details]
Patch
Comment 19 Gwang Yoon Hwang 2015-07-30 23:06:58 PDT
Comment on attachment 257837 [details]
Patch

Looks good to me!
Comment 20 Gyuyoung Kim 2015-08-03 07:20:47 PDT
(In reply to comment #19)
> Comment on attachment 257837 [details]
> Patch
> 
> Looks good to me!

Ossy, could you take a look this ?
Comment 21 Benjamin Poulain 2015-08-03 15:29:30 PDT
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.
Comment 22 WebKit Commit Bot 2015-08-03 18:17:10 PDT
Comment on attachment 257837 [details]
Patch

Clearing flags on attachment: 257837

Committed r187795: <http://trac.webkit.org/changeset/187795>
Comment 23 WebKit Commit Bot 2015-08-03 18:17:16 PDT
All reviewed patches have been landed.  Closing bug.