[webkit-reviews] review denied: [Bug 100819] Coordinated Graphics: Remove the dependency of ShareableSurface from Coordinated Graphics. : [Attachment 171998] Patch

[bugzilla-daemon at webkit.org]

Noam Rosenthal <noam.rosenthal at nokia.com> has denied Huang Dongsung
<luxtella at company100.net>'s request for review:
Bug 100819: Coordinated Graphics: Remove the dependency of ShareableSurface
from Coordinated Graphics.
https://bugs.webkit.org/show_bug.cgi?id=100819

Attachment 171998: Patch
https://bugs.webkit.org/attachment.cgi?id=171998&action=review

------- Additional Comments from Noam Rosenthal <noam.rosenthal at nokia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=171998&action=review


Recapping from IRC:
1. If the other implementation of CoordinatedSurface doesn't use IPC, why
create CoordinatedSurface::encode/decode?
2. Seems like the choice of implementations should be in compile time and not
runtime.

The more I look at this patch, the more I think that coordinated-graphics is
not the right way to make threaded compositing; I'd much prefer it if
TextureMapperLayer was in a different thread, and modify flush/updateContents
to use an actor model instead of a direct sync.

> Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedShareableSurface.h:39
> +    static PassRefPtr<CoordinatedShareableSurface>
create(CoordinatedSurfaceClient*, const WebCore::IntSize&, bool alpha);

Please use flags, not bool alpha.

> Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedSurface.h:61
> +    virtual CoordinatedSurfaceID id() const = 0;

This is only relevant to IPC

> Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedSurface.h:62
> +    virtual SurfaceType type() const = 0;

Isn't this a compile-time thing? When would you want to compile threaded and
IPC at the same time?

> Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedSurface.h:71
> +    virtual bool alpha() const = 0;
> +    virtual const WebCore::IntSize& size() const = 0;
> +
> +    // Create a graphics context that can be used to paint into the backing
store.
> +    virtual PassOwnPtr<WebCore::GraphicsContext> createGraphicsContext(const
WebCore::IntRect&) = 0;
> +
> +#if USE(TEXTURE_MAPPER)
> +    virtual void copyToTexture(PassRefPtr<WebCore::BitmapTexture>, const
WebCore::IntRect& target, const WebCore::IntPoint& sourceOffset) = 0;
> +#endif

So CoordinatedSurface is really either a ShareableSurface or something like a
thread-safe holder for an ImageBuffer.