[Webkit-unassigned] [Bug 108899] Coordinated Graphics : Refactor CoordinatedSurface to provide beginPaint and endPaint

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=108899





--- Comment #10 from Jae Hyun Park <jae.park at company100.net>  2013-02-12 18:47:37 PST ---
(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!

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.