Bug 104347 – Coordinated Graphics: Remove the dependency of WebCoordinatedSurface::Handle from Coordinated Graphics.

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Dongseong Hwang]

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.

[Dongseong Hwang]

Created attachment 178181 [details]
Patch

[Dongseong Hwang]

Created attachment 178342 [details]
Patch

[Dongseong Hwang]

Created attachment 178458 [details]
Patch

[Dongseong Hwang]

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.

[Noam Rosenthal]

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.

[Dongseong Hwang]

(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?

[Noam Rosenthal]

> 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.

[Dongseong Hwang]

Created attachment 178492 [details]
Patch

[Dongseong Hwang]

(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.

[Dongseong Hwang]

Created attachment 178932 [details]
Patch

[Dongseong Hwang]

rebased to upstream

[Noam Rosenthal]

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.

[Dongseong Hwang]

(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?

[Dongseong Hwang]

Created attachment 178985 [details]
Patch

[Noam Rosenthal]

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?

[Dongseong Hwang]

(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.

[Dongseong Hwang]

Created attachment 179375 [details]
Patch

[Dongseong Hwang]

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.

[Eric Seidel (no email)]

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.

[Dongseong Hwang]

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?

[Jocelyn Turcotte]

(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?

[Dongseong Hwang]

(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.

[WebKit Review Bot]

Comment on attachment 179375 [details]
Patch

Clearing flags on attachment: 179375

Committed r139525: <http://trac.webkit.org/changeset/139525>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.