Summary: | [CoordinatedGraphics] Rearrange uses of USE_COORDINATED_GRAPHICS_MULTIPROCESS | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Gyuyoung Kim <gyuyoung.kim> | ||||||||||||||||
Component: | New Bugs | Assignee: | Gyuyoung Kim <gyuyoung.kim> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | commit-queue, ossy, yoon | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Attachments: |
|
Description
Gyuyoung Kim
2015-07-24 00:54:50 PDT
Created attachment 257445 [details]
Patch
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. (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 ? (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. (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. Created attachment 257524 [details]
Patch
(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 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? Created attachment 257553 [details]
Patch
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 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. Created attachment 257741 [details]
Patch
Created attachment 257746 [details]
Patch
Yoon, for your information I succeed to build WebKit GTK with --threaded-compositor option. 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. Created attachment 257823 [details]
Patch
(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. Created attachment 257837 [details]
Patch
Comment on attachment 257837 [details]
Patch
Looks good to me!
(In reply to comment #19) > Comment on attachment 257837 [details] > Patch > > Looks good to me! Ossy, could you take a look this ? 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 on attachment 257837 [details] Patch Clearing flags on attachment: 257837 Committed r187795: <http://trac.webkit.org/changeset/187795> All reviewed patches have been landed. Closing bug. |