Bug 108899 – Coordinated Graphics : Refactor CoordinatedSurface to manage the lifecycle of GraphicsContext

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

[Jae Hyun Park]

WebKit1 CoordinatedSurface (I don't have the name for it yet. Any suggestions?) will be implemented using GraphicsSurface and ImageBuffer.

ImageBuffer owns GraphicsContext itself and cannot pass its OwnPtr around. Therefore, CoordinatedSurface should be refactored to pass GraphicsContextHolder and make other classes access GraphicsContext via GraphicsContextHolder. CoordinatedSurface will provide createGraphicsContextHolder method instead of createGraphicsContext method.

Also, methods that will be shared between WebCoordinatedSurface and WebKit1 CoordinatedSurface should be implemented in CoordinatedSurface.

[Jae Hyun Park]

Created attachment 186539 [details]
Patch

[Build Bot]

Comment on attachment 186539 [details]
Patch

Attachment 186539 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16375577

[Noam Rosenthal]

Comment on attachment 186539 [details]
Patch

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

> Source/WebCore/ChangeLog:20
> +        1. It introduces CoordinatedGraphics::GraphicsContextHolder. Other
> +        classes access GraphicsContext via GraphicsContextHolder, instead of
> +        accessing GraphicsContext directly.
> +
> +        WebKit1 CoordinatedSurface will be implemented using GraphicsSurface and
> +        ImageBuffer. ImageBuffer owns GraphicsContext itself and cannot pass its
> +        OwnPtr. Therefore, this patch refactors CoordinatedSurface to pass
> +        GraphicsContextHolder, and make other classes access GraphicsContext via
> +        GraphicsContextHolder.

This part doesn't make sense to me; Why not, instead, use explicit beginPaint that returns GraphicsContext* and endPaint?
We should find a more readable solution than the "holder" pattern...

[Jae Hyun Park]

(In reply to comment #3)
> This part doesn't make sense to me; Why not, instead, use explicit beginPaint that returns GraphicsContext* and endPaint?
> We should find a more readable solution than the "holder" pattern...

Thanks for the review!

In the current implementation, CoordinatedTile::updateBackBuffer() releases the PassOwnPtr<GraphicsContext> after using it.

IMHO, if we want to use beginPaint and endPaint pattern, we need to know CoordinatedSurface in CoordinatedTile, which is not pretty.
If we add a method such as UpdateAtlas::endPaintingOnAvailableBuffer, which will call endPaint in CoordinatedSurface, we still need to pass around UpdateAtlas.

I'm not sure which way is the right way to go. I'd really appreciate your comment noam :)

[Jae Hyun Park]

Created attachment 187820 [details]
Patch

[Noam Rosenthal]

Comment on attachment 187820 [details]
Patch

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

Please remove const& from uint32_t arguments.
Otherwise, LGTM but you need a sign off from a WK2 owner.

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:801
> +void CoordinatedGraphicsLayer::didEndContentUpdate(const uint32_t& atlasID)

no need for const&.

[Jae Hyun Park]

Created attachment 187823 [details]
Patch

[Benjamin Poulain]

Comment on attachment 187823 [details]
Patch

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

r- because of two very fishy things:
-m_graphicsContext is defined on CoordinatedSurface?
-Why is the context own by the CoordinatedSurface and not by the GraphicsSurface? The asymmetry is not clear to me.

First the first, I think the ChangeLog should explain it if it is correct. For the second, some context here is enough, I just don't know much about CoordinatedGraphics.

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:794
> +GraphicsContext* CoordinatedGraphicsLayer::beginContentUpdate(const IntSize& size, uint32_t& atlas, IntPoint& offset)

atlas being an uint32_t, shouldn't the argument be named atlasID?

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.h:74
> +    virtual GraphicsContext* beginContentUpdate(const IntSize&, CoordinatedSurface::Flags, uint32_t& atlasID, IntPoint&) = 0;

It is okay to have the IntPoint named here.
We don't put the attribute names when they are obvious.

Depending on the kind of API you are making, it should not have one output by return and 2 by passing arguments.
I don't know what are the arguments for so I let you check that.

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedSurface.h:61
> +    OwnPtr<GraphicsContext> m_graphicsContext;

Why is this protected of CoordinatedSurface?
CoordinatedSurface defines an interface, it should not force the way it is implemented.

> Source/WebKit2/Shared/CoordinatedGraphics/WebCoordinatedSurface.cpp:109
> -    if (isBackedByGraphicsSurface())
> -        return m_graphicsSurface->beginPaint(rect, 0 /* Write without retaining pixels*/);
> +    if (isBackedByGraphicsSurface()) {
> +        m_graphicsContext = m_graphicsSurface->beginPaint(rect, 0 /* Write without retaining pixels*/);

The 0 + comment are mysterious.

[Jae Hyun Park]

Created attachment 187989 [details]
Patch

[Jae Hyun Park]

(In reply to comment #8)
> (From update of attachment 187823 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=187823&action=review
> 
> r- because of two very fishy things:
> -m_graphicsContext is defined on CoordinatedSurface?
> -Why is the context own by the CoordinatedSurface and not by the GraphicsSurface? The asymmetry is not clear to me.
> 
> First the first, I think the ChangeLog should explain it if it is correct. For the second, some context here is enough, I just don't know much about CoordinatedGraphics.
> 
GraphicsContext is owned by the CoordinatedSurface because there are two ways to create GraphicsContext in CoordinatedSurface, via GraphicsSurface and ShareableBitmap. Both GraphicsSurface and ShareableBitmap provide PassOwnPtr<GraphicsContext> and CoordinatedSurface manages lifecycle of that GraphicsContext by providing beginPaint and endPaint.

This refactoring is necessary because in Threaded Coordinated Graphics, CoordinatedSurface will provide GraphicsContext using either GraphicsSurface or ImageBuffer. Since ImageBuffer owns GraphicsContext itself, CoordinatedSurface cannot pass PassOwnPtr<GraphicsContext>. Instead, it should pass GraphicsContext* so that CoordinatedImageBacking and UpdateAtlas can use the GraphicsContext.

> > Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:794
> > +GraphicsContext* CoordinatedGraphicsLayer::beginContentUpdate(const IntSize& size, uint32_t& atlas, IntPoint& offset)
> 
> atlas being an uint32_t, shouldn't the argument be named atlasID?
> 
Done.

> > Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.h:74
> > +    virtual GraphicsContext* beginContentUpdate(const IntSize&, CoordinatedSurface::Flags, uint32_t& atlasID, IntPoint&) = 0;
> 
> It is okay to have the IntPoint named here.
> We don't put the attribute names when they are obvious.
> 
> Depending on the kind of API you are making, it should not have one output by return and 2 by passing arguments.
> I don't know what are the arguments for so I let you check that.
> 
Done.

> > Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedSurface.h:61
> > +    OwnPtr<GraphicsContext> m_graphicsContext;
> 
> Why is this protected of CoordinatedSurface?
> CoordinatedSurface defines an interface, it should not force the way it is implemented.
> 
Now that I think about it, it should be in WebCoordinatedSurface.h

> > Source/WebKit2/Shared/CoordinatedGraphics/WebCoordinatedSurface.cpp:109
> > -    if (isBackedByGraphicsSurface())
> > -        return m_graphicsSurface->beginPaint(rect, 0 /* Write without retaining pixels*/);
> > +    if (isBackedByGraphicsSurface()) {
> > +        m_graphicsContext = m_graphicsSurface->beginPaint(rect, 0 /* Write without retaining pixels*/);
> 
> The 0 + comment are mysterious.

The comments are about lock options. When GraphicsSurface::beginPaint is called, it takes lockOption argument, and calls platformLock(), which will lock the surface with the passed option. 
The comment just describes the lock option.

Thank you for your review!

[Noam Rosenthal]

Comment on attachment 187989 [details]
Patch

Wouldn't it make more sense to refactor ImageBuffer to be able to return PassOwnPtr<GraphicsContext>, rather than work around it here? Benjamin, what do you think?

[Benjamin Poulain]

(In reply to comment #11)
> (From update of attachment 187989 [details])
> Wouldn't it make more sense to refactor ImageBuffer to be able to return PassOwnPtr<GraphicsContext>, rather than work around it here? Benjamin, what do you think?

I don't really like the idea. It is easier to maintain correctness with the current design:
-The GraphicsContext cannot outlive ImageBuffer.
-Having multiple GraphicsContext or a ref-counted GraphicsContext would lead to awful situations.

[Benjamin Poulain]

> GraphicsContext is owned by the CoordinatedSurface because there are two ways to create GraphicsContext in CoordinatedSurface, via GraphicsSurface and ShareableBitmap. Both GraphicsSurface and ShareableBitmap provide PassOwnPtr<GraphicsContext> and CoordinatedSurface manages lifecycle of that GraphicsContext by providing beginPaint and endPaint.
> 
> This refactoring is necessary because in Threaded Coordinated Graphics, CoordinatedSurface will provide GraphicsContext using either GraphicsSurface or ImageBuffer. Since ImageBuffer owns GraphicsContext itself, CoordinatedSurface cannot pass PassOwnPtr<GraphicsContext>. Instead, it should pass GraphicsContext* so that CoordinatedImageBacking and UpdateAtlas can use the GraphicsContext.

Let's ask things differently, why doesn't GraphicsSurface have the responsibility for its GraphicsContext in the first place?

[Noam Rosenthal]

(In reply to comment #13)
> Let's ask things differently, why doesn't GraphicsSurface have the responsibility for its GraphicsContext in the first place?

The idea was that releasing the GraphicsContext would trigger an unlock.
But I don't mind changing that behavior to have a GraphicsSurface::endPaint() and change beginPaint() to return a raw pointer.

[Jae Hyun Park]

Created attachment 188836 [details]
Patch

[Jae Hyun Park]

(In reply to comment #13)

> Let's ask things differently, why doesn't GraphicsSurface have the responsibility for its GraphicsContext in the first place?

I updated the patch trying to make GraphicsSurface and ShareableBitmap have the responsibilities for their own GraphicsContext, but noam thought it was quite messy.

The main purpose of this patch is to make CoordinatedSurface pass GraphicsContext*, and the problem is that ShareableBitmap doesn't own its GraphicsContext and ImageBuffer does.

// the following is a talk with noam.
[20:14] <noamr> jaepark: the problem is - ShareableBitmap doesn't own its graphicsContext; ImageBuffer does. We're trying to use a class that has both backends
[20:15] <noamr> so the solution is either to make ImageBuffer not own its GraphicsContext, or make ShareableBitmap own its GraphicsContext, or work around the issue in the API
[20:15] <jaepark> right.
[20:15] <noamr> I don't particularly care which one it is, but since benjaminp had some issues with option (3), I'd like to hear his thoughts.

So I provided a way to work around the issue in the API in the second last patch, which noamr thought was reasonable.

I'd really appreciate your comment on this benjamin. :)

[Simon Fraser (smfr)]

Comment on attachment 188836 [details]
Patch

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

> Source/WebCore/platform/graphics/surfaces/GraphicsSurface.cpp:52
> +    m_graphicsContexts.append(createGraphicsContext(rect.size(), bits, stride));

It seems wrong to be able to create more than one GraphicsContext for a given surface. What happens when painting is interleaved from the two contexts?

> Source/WebCore/platform/graphics/surfaces/GraphicsSurface.h:88
> +    GraphicsContext* beginPaint(const IntRect&, LockOptions);
> +    void endPaint();

Why not just create a GraphicsContext* when first asked, then keep it around? Forcing clients to call endPaint() is a bit burdensome.

> Source/WebKit2/Shared/ShareableBitmap.h:173
> +    Vector<OwnPtr<WebCore::GraphicsContext> > m_graphicsContexts;

Same comment here. Why allow more than one context?

[Jae Hyun Park]

(In reply to comment #17)
> (From update of attachment 188836 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=188836&action=review
> 
> > Source/WebCore/platform/graphics/surfaces/GraphicsSurface.cpp:52
> > +    m_graphicsContexts.append(createGraphicsContext(rect.size(), bits, stride));
> 
> It seems wrong to be able to create more than one GraphicsContext for a given surface. What happens when painting is interleaved from the two contexts?
> 
> > Source/WebCore/platform/graphics/surfaces/GraphicsSurface.h:88
> > +    GraphicsContext* beginPaint(const IntRect&, LockOptions);
> > +    void endPaint();
> 
> Why not just create a GraphicsContext* when first asked, then keep it around? Forcing clients to call endPaint() is a bit burdensome.

Forcing clients to call endPaint() triggers platformUnlock() to be called. Since we platformLock() at beginPaint(), we should call platformUnlock(). So endPaint() must be called each time beginPaint() is called.

> 
> > Source/WebKit2/Shared/ShareableBitmap.h:173
> > +    Vector<OwnPtr<WebCore::GraphicsContext> > m_graphicsContexts;
> 
> Same comment here. Why allow more than one context?

I was also surprised that GraphicsContext is created more than once for a given surface. But in CoordinatedTile::updateBackBuffer, we first call m_client->beginContentUpdate(), which will create a GraphicsContext, and then m_tiledBackingStore->client()->tiledBackingStorePaint() will call CoordinatedTile::updateBackbuffer after a long series of function calls, which will then call another m_client->beginContentUpdate() that creates another GraphicsContext.

The below is the stack trace for above behavior.

ASSERTION FAILED: m_graphicsContexts.size() == 1
/home/jaepark/workspace/WebKitEfl/Source/WebKit2/Shared/ShareableBitmap.cpp(174) : WebCore::GraphicsContext* WebKit::ShareableBitmap::beginPaint()
1   0x7f6528304b0b WebKit::ShareableBitmap::beginPaint()
2   0x7f652834af92 WebKit::WebCoordinatedSurface::beginPaint(WebCore::IntRect const&)
3   0x7f652416c520 WebCore::UpdateAtlas::beginPaintingOnAvailableBuffer(unsigned int&, WebCore::IntSize const&, WebCore::IntPoint&)
4   0x7f6528527a66 WebKit::CoordinatedLayerTreeHost::beginContentUpdate(WebCore::IntSize const&, unsigned int, unsigned int&, WebCore::IntPoint&)
5   0x7f65241542f7 WebCore::CoordinatedGraphicsLayer::beginContentUpdate(WebCore::IntSize const&, unsigned int&, WebCore::IntPoint&)
6   0x7f652416b835 WebCore::CoordinatedTile::updateBackBuffer()
7   0x7f6524126521 WebCore::TiledBackingStore::updateTileBuffers()
8   0x7f6524127483 WebCore::TiledBackingStore::createTiles()
9   0x7f6524126170 WebCore::TiledBackingStore::coverWithTilesIfNeeded()
10  0x7f6524126aab WebCore::TiledBackingStore::commitScaleChange()
11  0x7f6524126a5a WebCore::TiledBackingStore::setContentsScale(float)
12  0x7f6524153c23 WebCore::CoordinatedGraphicsLayer::createBackingStore()
13  0x7f65241547d6 WebCore::CoordinatedGraphicsLayer::updateContentBuffers()
14  0x7f652415370b WebCore::CoordinatedGraphicsLayer::flushCompositingStateForThisLayerOnly()
15  0x7f6524152b1c WebCore::CoordinatedGraphicsLayer::flushCompositingState(WebCore::FloatRect const&)
16  0x7f6524152b5d WebCore::CoordinatedGraphicsLayer::flushCompositingState(WebCore::FloatRect const&)
17  0x7f6524152b5d WebCore::CoordinatedGraphicsLayer::flushCompositingState(WebCore::FloatRect const&)
18  0x7f6524152b5d WebCore::CoordinatedGraphicsLayer::flushCompositingState(WebCore::FloatRect const&)
19  0x7f6524152b5d WebCore::CoordinatedGraphicsLayer::flushCompositingState(WebCore::FloatRect const&)
20  0x7f6524352e2f WebCore::RenderLayerCompositor::flushPendingLayerChanges(bool)
21  0x7f6523fee651 WebCore::FrameView::flushCompositingStateForThisFrame(WebCore::Frame*)
22  0x7f6523ff793a WebCore::FrameView::paintContents(WebCore::GraphicsContext*, WebCore::IntRect const&)
23  0x7f6524094e67 WebCore::ScrollView::paint(WebCore::GraphicsContext*, WebCore::IntRect const&)
24  0x7f652440ef52 WebCore::RenderWidget::paint(WebCore::PaintInfo&, WebCore::LayoutPoint const&)
25  0x7f65243322ad WebCore::RenderLayer::paintLayerContents(WebCore::GraphicsContext*, WebCore::RenderLayer::LayerPaintingInfo const&, unsigned int)
26  0x7f652434fce6 WebCore::RenderLayerBacking::paintIntoLayer(WebCore::GraphicsLayer const*, WebCore::GraphicsContext*, WebCore::IntRect const&, unsigned int, unsigned int)
27  0x7f652435005f WebCore::RenderLayerBacking::paintContents(WebCore::GraphicsLayer const*, WebCore::GraphicsContext&, unsigned int, WebCore::IntRect const&)
28  0x7f65241038bd WebCore::GraphicsLayer::paintGraphicsLayerContents(WebCore::GraphicsContext&, WebCore::IntRect const&)
29  0x7f6524153c71 WebCore::CoordinatedGraphicsLayer::tiledBackingStorePaint(WebCore::GraphicsContext*, WebCore::IntRect const&)
30  0x7f652416b973 WebCore::CoordinatedTile::updateBackBuffer()
31  0x7f6524126521 WebCore::TiledBackingStore::updateTileBuffers()

I'm still not sure if this is the right direction. IMHO, the second last patch (https://bugs.webkit.org/attachment.cgi?id=187989&action=review) would be more appropriate for this refactoring. How do you feel about that?

[Build Bot]

Comment on attachment 188836 [details]
Patch

Attachment 188836 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16619004

New failing tests:
http/tests/inspector/appcache/appcache-manifest-with-non-existing-file.html

[Jae Hyun Park]

I've been working on this bug for a while now, and I still don't see a proper direction.

The purpose of this refactoring is to use ThreadSafeCoordinatedSurface (which will be used in Threaded Coordinated Graphics). I am planning to implement ThreadSafeCoordinatedSurface to use GraphicsSurface and ImageBuffer as a backend (WIP patch https://bugs.webkit.org/show_bug.cgi?id=109661). Since ImageBuffer only provides GraphicsContext* to get GraphicsContext whereas ShareableBitmap and GraphicsSurface provides PassOwnPtr<GraphicsContext>, a refactoring was needed.

Over the past few weeks, the reviewers, noamr, benjaminp, and smfr, have suggested several ways. I have tried them all but each had problems (some were big, some were trivial).

During the refactoring, I found an abnormal behavior in which GraphicsContext is created twice through recursive calls before releasing even one GraphicsContext (this behavior occurs only in iframe cases). The call stack can be found in https://bugs.webkit.org/show_bug.cgi?id=108899#c18. Due to this behavior, the class that manages GraphicsContext, whether it be GraphicsSurface, ShareableBitmap, or even CoordinatedSurface, must hold GraphicsContext in a stack, which is pretty ugly. 

I have tried 3 different approaches in refactoring.
1. GraphicsContextHolder pattern.
-> Rejected by noamr, since "holder" pattern makes the code hard to read.

2. beginPaint & endPaint pattern where CoordinatedSurface manages GraphicsContext.
-> Rejected by benjaminp. CoordinatedSurface managing GraphicsContext is not reasonable.

3. beginPaint & endPaint pattern where GraphicsSurface & ShareableBitmap manages GraphicsContext.
-> Rejected by smfr. GraphicsSurface and ShareableBitmap managing GraphicsContext as a stack is unreasonable. Also, beginPaint and endPaint pattern is burdensome.

More importantly, all three cases must hold GraphicsContext in a stack due to the abnormal behavior I mentioned above.

I have dug further about this behavior, and also discussed with noamr and dshuang about it. Noam thought this can be solved by triggering a flush directly using TBS timer instead of triggering a paint for the particular tile. However, since Coordinated Graphics doesn't use TBS timer, I think this cannot be a solution. The problem of this is that two different CoordinatedTile can use the same UpdateAtlas. 

I thought about the ways to move this patch forward.

1. Add an API in ImageBuffer that creates a new GraphicsContext and returns as PassOwnPtr<GraphicsContext> (like current behavior in GraphicsSurface).
-> This is the most proper approach that I can think of right now, even though it may not be the best approach.

2. Make CoordinatedTile use separate UpdateAtlas.
-> This is practically impossible since it will dramatically increase the memory usage.

3. Allow GraphicsContext to be managed in Vector form.
-> IMHO, this is unacceptable since it is clearly wrong.

4. Forbid sub-layers to flush when the root layer is performing flush.
-> The GraphicsContext is created 2 times in a row before even releasing one because the root layer flush is calling the sub layers to flush. However, I'm not even sure if this behavior is wrong. As I mentioned above, I think the problem is different CoordinatedTile sharing an UpdateAtlas. 

I could really use some help. Please leave comments/thoughts. 

Thanks.

[Simon Fraser (smfr)]

How about (with fake function names)

class ShareableBitmapClient  {
  virtual drawBuffer(ShareableBitmap*, GraphicsContext*) = 0;
}

ShareableBitmap::drawContents()
{
  OwnPtr<GraphicsContext> context = create.....
  client->drawBuffer(this, context);
}

And there would be no other way to get the GraphicsContext for a bitmap.

That way there is zero ambiguity about the GraphicsContext ownership.

[Jae Hyun Park]

(In reply to comment #21)
> How about (with fake function names)
> 
> class ShareableBitmapClient  {
>   virtual drawBuffer(ShareableBitmap*, GraphicsContext*) = 0;
> }
> 
> ShareableBitmap::drawContents()
> {
>   OwnPtr<GraphicsContext> context = create.....
>   client->drawBuffer(this, context);
> }
> 
> And there would be no other way to get the GraphicsContext for a bitmap.
> 
> That way there is zero ambiguity about the GraphicsContext ownership.

Thanks for the comment!

However, I am not sure how this pattern solves the current problem.
I don't see a difference from just passing OwnPtr<GraphicsContext>.

The core of this refactoring is to prepare for implementing ThreadSafeCoordinatedSurface, which has ImageBuffer/GraphicsSurface as a backend.

Are you against the first approach I mentioned?
1. Add an API in ImageBuffer that creates a new GraphicsContext and returns as PassOwnPtr<GraphicsContext> (like current behavior in GraphicsSurface).
-> This is the most proper approach that I can think of right now, even though it may not be the best approach.

[Jae Hyun Park]

After a discussion with noam, I decided to fix the bug (GraphicsContext created twice through a recursive call) first.

[Noam Rosenthal]

Comment on attachment 188836 [details]
Patch

Clearing review flags as per comments.
JaeHyun, are you still working on this?

[Jae Hyun Park]

(In reply to comment #24)
> (From update of attachment 188836 [details])
> Clearing review flags as per comments.
> JaeHyun, are you still working on this?

I'm sorry but I can't work on this for personal reasons. ryumiel will take over this bug.

[Jae Hyun Park]

Created attachment 203784 [details]
Patch

Rebased to upstream.

[Jae Hyun Park]

(In reply to comment #23)
> After a discussion with noam, I decided to fix the bug (GraphicsContext created twice through a recursive call) first.

This bug was resolved in https://bugs.webkit.org/show_bug.cgi?id=117222

[Kalyan]

Comment on attachment 203784 [details]
Patch

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

> Source/WebKit2/Shared/CoordinatedGraphics/WebCoordinatedSurface.cpp:-196
> -            uint32_t textureTarget = textureGL->textureTarget();

Why move graphicssurface related things to CoordinateSurface ?

[Jae Hyun Park]

(In reply to comment #28)
> (From update of attachment 203784 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=203784&action=review
> 
> > Source/WebKit2/Shared/CoordinatedGraphics/WebCoordinatedSurface.cpp:-196
> > -            uint32_t textureTarget = textureGL->textureTarget();
> 
> Why move graphicssurface related things to CoordinateSurface ?

Actually, this part is to prepare for CoordinatedSurface to be used in WebKit1. I will separate this part to another patch.

[Kalyan]

Comment on attachment 203784 [details]
Patch

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

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedSurface.cpp:67
> +        GraphicsSurface::SupportsSoftwareWrite

Why not move the graphics surface creation to CoordinatedSurface constructor. Seems unnecessary that we create the GraphicsSurface and than pass it to WebCoordinatedSurface/CoordinatedSurface constructor.

[Kalyan]

(In reply to comment #29)
> (In reply to comment #28)
> > (From update of attachment 203784 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=203784&action=review
> > 
> 
> Actually, this part is to prepare for CoordinatedSurface to be used in WebKit1. I will separate this part to another patch.

Interesting, so what is it going to be used for ?? (I mean the usage of GraphicsSurface)

[Jae Hyun Park]

(In reply to comment #21)
> How about (with fake function names)
> 
> class ShareableBitmapClient  {
>   virtual drawBuffer(ShareableBitmap*, GraphicsContext*) = 0;
> }
> 
> ShareableBitmap::drawContents()
> {
>   OwnPtr<GraphicsContext> context = create.....
>   client->drawBuffer(this, context);
> }
> 
> And there would be no other way to get the GraphicsContext for a bitmap.
> 
> That way there is zero ambiguity about the GraphicsContext ownership.

It is difficult to apply ShareableBitmap client pattern to CoordinatedSurface because when CoordinatedTile requests for GraphicsContext, the GraphicsContext is modified in WebCoordinatedSurface and UpdateAtlas. Then, CoordinatedTile modifies this GraphicsContext (translate and scale), and then passes to TiledBackingStoreClient for actual painting. Therefore, it is really hard to apply this client pattern.

[Jae Hyun Park]

(In reply to comment #31)
> 
> Interesting, so what is it going to be used for ?? (I mean the usage of GraphicsSurface)

CoordinatedSurface for WebKit1 will not need GraphicSurface. So apparently this patch is not valid. CoordinatedSurface for WebKit1 is needed for Threaded Coordinated Graphics (https://bugs.webkit.org/show_bug.cgi?id=100341).

[Jae Hyun Park]

(In reply to comment #32)

> 
> It is difficult to apply ShareableBitmap client pattern to CoordinatedSurface because when CoordinatedTile requests for GraphicsContext, the GraphicsContext is modified in WebCoordinatedSurface and UpdateAtlas. Then, CoordinatedTile modifies this GraphicsContext (translate and scale), and then passes to TiledBackingStoreClient for actual painting. Therefore, it is really hard to apply this client pattern.

After a discussion with noam, I will try this client pattern.

[Jae Hyun Park]

Created attachment 203908 [details]
Patch

[EFL EWS Bot]

Comment on attachment 203908 [details]
Patch

Attachment 203908 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/747872

[Jae Hyun Park]

Created attachment 203911 [details]
Patch

Fixed EFL build

[Gwang Yoon Hwang]

(In reply to comment #37)
> Created an attachment (id=203911) [details]
> Patch
> 
> Fixed EFL build

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

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedSurface.h:57
>      // Create a graphics context that can be used to paint into the backing store.

This comment is going to be invalid.

> Source/WebCore/platform/graphics/texmap/coordinated/UpdateAtlas.cpp:98
>      // No available buffer was found, returning null.

Ditto.

[Noam Rosenthal]

Comment on attachment 203911 [details]
Patch

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

LGTM with nitpicks

> Source/WebCore/ChangeLog:12
> +        CoordinatedImageBacking and UpdateAtlas does not asks for the ownership

does not asks -> do not ask

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedTile.h:81
> +    virtual bool paintToSurface(const IntSize&, uint32_t& /* atlasID */, IntPoint&, CoordinatedSurface::Client*) = 0;

No need to comment atlasID

> Source/WebCore/platform/graphics/texmap/coordinated/UpdateAtlas.cpp:57
> +

Remove new line

> Source/WebCore/platform/graphics/texmap/coordinated/UpdateAtlas.h:49
> +    bool paintOnAvailableBuffer(const IntSize&, uint32_t& /* atlasID */, IntPoint& /* offset */, CoordinatedSurface::Client*);

No need to comment out parameter names

> Source/WebKit2/Shared/CoordinatedGraphics/WebCoordinatedSurface.cpp:190
> +    if (!client)
> +        return;
> +

We should actually assert for the client.

[Jae Hyun Park]

Created attachment 203913 [details]
Patch

[Jae Hyun Park]

(In reply to comment #38)
> (In reply to comment #37)
> > Created an attachment (id=203911) [details] [details]
> > Patch
> > 
> > Fixed EFL build
> 
> View in context: https://bugs.webkit.org/attachment.cgi?id=203911&action=review
> 
> > Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedSurface.h:57
> >      // Create a graphics context that can be used to paint into the backing store.
> 
> This comment is going to be invalid.

Fixed.

> 
> > Source/WebCore/platform/graphics/texmap/coordinated/UpdateAtlas.cpp:98
> >      // No available buffer was found, returning null.
> 
> Ditto.

Fixed.

Thanks for the review!

[Jae Hyun Park]

(In reply to comment #39)
> (From update of attachment 203911 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=203911&action=review
> 
> LGTM with nitpicks
> 
> > Source/WebCore/ChangeLog:12
> > +        CoordinatedImageBacking and UpdateAtlas does not asks for the ownership
> 
> does not asks -> do not ask
Fixed.

> 
> > Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedTile.h:81
> > +    virtual bool paintToSurface(const IntSize&, uint32_t& /* atlasID */, IntPoint&, CoordinatedSurface::Client*) = 0;
> 
> No need to comment atlasID
Fixed.

> 
> > Source/WebCore/platform/graphics/texmap/coordinated/UpdateAtlas.cpp:57
> > +
> 
> Remove new line
Fixed.

> 
> > Source/WebCore/platform/graphics/texmap/coordinated/UpdateAtlas.h:49
> > +    bool paintOnAvailableBuffer(const IntSize&, uint32_t& /* atlasID */, IntPoint& /* offset */, CoordinatedSurface::Client*);
> 
> No need to comment out parameter names
Fixed.

> 
> > Source/WebKit2/Shared/CoordinatedGraphics/WebCoordinatedSurface.cpp:190
> > +    if (!client)
> > +        return;
> > +
> 
> We should actually assert for the client.
Fixed.

Thanks for the review!

[WebKit Commit Bot]

Comment on attachment 203913 [details]
Patch

Clearing flags on attachment: 203913

Committed r151262: <http://trac.webkit.org/changeset/151262>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.