It is a preparation patch for Threaded Coordinated Graphics on WK1. Currently, UpdateAtlas and CoordinatedImageBacking use WebCoordinatedSurface::Handle, but WebCoordinatedSurface::Handle can be used only IPC-based Coordinated Graphics. So this patch removes the dependency of WebCoordinatedSurface::Handle from UpdateAtlas and CoordinatedImageBacking. The constructor of WebCoordinatedSurface creates a handle implicitly and then destroys the handle as soon as sending the handle to UI Process. External class cannot create WebCoordinatedSurface::Handle anymore, because we don't have the requirement of creating many handles.
Created attachment 178181 [details] Patch
Created attachment 178342 [details] Patch
Created attachment 178458 [details] Patch
Comment on attachment 178458 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=178458&action=review > Source/WebKit2/Shared/CoordinatedGraphics/WebCoordinatedSurface.cpp:238 > + m_releaseHandleTimer = adoptPtr(new Timer<WebCoordinatedSurface>(const_cast<WebCoordinatedSurface*>(this), &WebCoordinatedSurface::releaseHandleTimerFired)); This patch creates the timer lazily because a WebCoordinatedSurface instance in UI Process does not need to create the timer. Moreover, the destructor of TimerBase has ASSERT(m_thread == currentThread()), so UI Process can fire the Assertion if WebCoordinatedSurface is destroyed in the compositing thread.
Comment on attachment 178458 [details] Patch I don't see the point in this. You can remove the dependency of the Handle in CoordinatedImageBacking and UpdateAtlas by converting the handle in LayerTreeCoordinator and LayerTreeCoordinatorProxy. I'd rather we don't add this kind of magic in the argument coders.
(In reply to comment #5) > (From update of attachment 178458 [details]) > I don't see the point in this. > You can remove the dependency of the Handle in CoordinatedImageBacking and UpdateAtlas by converting the handle in LayerTreeCoordinator and LayerTreeCoordinatorProxy. > I'd rather we don't add this kind of magic in the argument coders. My purpose is to share the same API with both IPC-base and Threaded Coordinated Graphics. In the detail, Threaded Coordinated Graphics will use LayerTreeCoordinatorProxy also, although LayerTreeCoordinatorProxy will be refactored to extract subset of LayerTreeCoordinatorProxy that Threaded Coordinated Graphics needs. If we convert the handle in LayerTreeCoordinator, we will have following code. #if ENABLE(IPC_BASED_COORDIATED_GRAPHICS) void LayerTreeCoordinatorProxy::updateImageBacking(CoordinatedImageBackingID, const WebCoordinatedSurface::Handle&); #elif ENABLE(THREADED_COORDIATED_GRAPHICS) void LayerTreeCoordinatorProxy::updateImageBacking(CoordinatedImageBackingID, PassRefPtr<CoordinatedSurface>); #endif If we cast the magic in ArgumentCoder, we can have the same API. void LayerTreeCoordinatorProxy::updateImageBacking(CoordinatedImageBackingID, PassRefPtr<CoordinatedSurface>); I think both have its own benefit. I think it's a matter of personal taste. Do you prefer the first option?
> If we cast the magic in ArgumentCoder, we can have the same API. > void LayerTreeCoordinatorProxy::updateImageBacking(CoordinatedImageBackingID, PassRefPtr<CoordinatedSurface>); > > I think both have its own benefit. I think it's a matter of personal taste. Do you prefer the first option? I prefer it if there was some class that does the IPC-specific stuff, and not try to make everything magically work the same for IPC/threading, since it's not really 100% the same. I prefer it if there was a supervisor/coordinator class that was shared between IPC/threading, and then a smaller "client" class that does the IPC-specific/threading-specific thing, which is where the handle conversion would take place.
Created attachment 178492 [details] Patch
(In reply to comment #7) > I prefer it if there was some class that does the IPC-specific stuff, and not try to make everything magically work the same for IPC/threading, since it's not really 100% the same. > > I prefer it if there was a supervisor/coordinator class that was shared between IPC/threading, and then a smaller "client" class that does the IPC-specific/threading-specific thing, which is where the handle conversion would take place. Ok, I understand.
Created attachment 178932 [details] Patch
rebased to upstream
Comment on attachment 178932 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=178932&action=review > Source/WebKit2/Shared/CoordinatedGraphics/WebCoordinatedSurface.h:125 > + > + // Only WebCoordinatedSurface in Web Process has this member. > + OwnPtr<Handle> m_handle; > + mutable OwnPtr<WebCore::Timer<WebCoordinatedSurface> > m_releaseHandleTimer; > + > +#ifndef NDEBUG > + enum Purpose { > + Producer, > + Consumer > + }; > + // This member exists for only a debugging purpose. It is Producer in Web Process, while Consumer in UI Process. > + Purpose m_purpose; > +#endif This is not really needed. You can do everything else in this patch without having this magically-released handle.
(In reply to comment #12) > (From update of attachment 178932 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=178932&action=review > > > Source/WebKit2/Shared/CoordinatedGraphics/WebCoordinatedSurface.h:125 > > + > > + // Only WebCoordinatedSurface in Web Process has this member. > > + OwnPtr<Handle> m_handle; > > + mutable OwnPtr<WebCore::Timer<WebCoordinatedSurface> > m_releaseHandleTimer; > > + > > +#ifndef NDEBUG > > + enum Purpose { > > + Producer, > > + Consumer > > + }; > > + // This member exists for only a debugging purpose. It is Producer in Web Process, while Consumer in UI Process. > > + Purpose m_purpose; > > +#endif > > This is not really needed. > You can do everything else in this patch without having this magically-released handle. Ok, I'll remain createHandle(Handle&) public. And remove Purpose enum. Do I understand correctly what you mean?
Created attachment 178985 [details] Patch
Comment on attachment 178985 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=178985&action=review Note that I r+/cq- those patches. Please work with guys at #qtwebkit to make sure you submit those patches gradually without breaking anything. > Source/WebKit2/ChangeLog:15 > + CoordinatedImageBacking. Now CoordinatedLayerTreeHost treats with the > + Handle. treats with the handles -> converts the handle to a WebCoordinatedSurface. > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedImageBacking.cpp:118 > + // If failing to send the message, try again in the next update. If sending the message fails > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/UpdateAtlas.cpp:45 > + // FIXME: We need to find more elegant way. > + if (!m_client->createUpdateAtlas(m_ID, m_surface)) > + m_surface.clear(); What's not elegant?
(In reply to comment #15) > (From update of attachment 178985 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=178985&action=review Thank you for review! > Note that I r+/cq- those patches. Please work with guys at #qtwebkit to make sure you submit those patches gradually without breaking anything. I'll ask jturcotte to review too. > > Source/WebKit2/ChangeLog:15 > > + CoordinatedImageBacking. Now CoordinatedLayerTreeHost treats with the > > + Handle. > > treats with the handles -> converts the handle to a WebCoordinatedSurface. > > > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedImageBacking.cpp:118 > > + // If failing to send the message, try again in the next update. > > If sending the message fails Thx. > > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/UpdateAtlas.cpp:45 > > + // FIXME: We need to find more elegant way. > > + if (!m_client->createUpdateAtlas(m_ID, m_surface)) > > + m_surface.clear(); > > What's not elegant? Currently, if we fail to create handle, the UpdateAtlas gives up drawing anything. The comment means that we can do more proactive handling.. How about changing comment like // FIXME: Currently, if we fail to send the surface to UI Process, UpdateAtlas gives up drawing anything implicitly.
Created attachment 179375 [details] Patch
Comment on attachment 179375 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=179375&action=review > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/UpdateAtlas.cpp:43 > + // FIXME: Currently, if sending the message fails, UpdateAtlas gives up drawing anything implicitly. Changed the comment.
Comment on attachment 178985 [details] Patch Cleared Noam Rosenthal's review+ from obsolete attachment 178985 [details] so that this bug does not appear in http://webkit.org/pending-commit.
jocelyn : as you mentioned http://webkit.org/b/102994#c14, we want that Qt WK1 reuse Coordinated Graphics. This patch is needed for WK1 to reuse CoordinatedSurface. As noam mentioned #c15, I need your opinion on this project. Do you think this patch can be landed?
(In reply to comment #20) > jocelyn : as you mentioned http://webkit.org/b/102994#c14, we want that Qt WK1 reuse Coordinated Graphics. > > This patch is needed for WK1 to reuse CoordinatedSurface. As noam mentioned #c15, I need your opinion on this project. > Do you think this patch can be landed? The patch looks good to me, nothing blasphemous here. I'm okay with the general direction, just make sure that you track regressions and fix them in a reasonable delay. I'm still not sure what's the status of the WebKit2 lockout, is this considered a trivial change or not?
(In reply to comment #21) > The patch looks good to me, nothing blasphemous here. > I'm okay with the general direction, just make sure that you track regressions and fix them in a reasonable delay. I'm glad to hear you :) I'll do my best not to make regression. If so, I'll fix immediately. > I'm still not sure what's the status of the WebKit2 lockout, is this considered a trivial change or not? The term of trivial change is so ambiguous. IMO coordinated graphics is on WK2 by chance. It is not strange if coordinated graphics is on WK1 or Platform. So it seems to be ok.
Comment on attachment 179375 [details] Patch Clearing flags on attachment: 179375 Committed r139525: <http://trac.webkit.org/changeset/139525>
All reviewed patches have been landed. Closing bug.