Bug 100819 – Coordinated Graphics: Remove the dependency of ShareableSurface from Coordinated Graphics.

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

[Dongseong Hwang]

It is a preparation patch for Threaded Coordinated Graphics.

Currently, LayerTreeCoordinator, CoordinatedGraphicsLayer, CoordinatedTile and
CoordinatedBackingStore directly use ShareableSurface. ShareableSurface was
implemented to share an image between processes. We do not want to put many
guards on those classes to implement Threaded Coordinated Graphics.

This patch introduces CoordinatedSurface. Those classes just use
CoordinatedSurface. CoordinatedShareableSurface plays a role in sharing an image
between processes as ShareableSurface did. The implementation of
CoordinatedShareableSurface is just using ShareableSurface.

We will introduce CoordinatedThreadSafeSurface, which plays a role in sharing an
image between threads. It will be implemented using mutex.

[Dongseong Hwang]

Created attachment 171588 [details]
Patch

[Dongseong Hwang]

Created attachment 171774 [details]
Patch

[Dongseong Hwang]

(In reply to comment #2)
> Created an attachment (id=171774) [details]
> Patch

Rebased to upstream

[Dongseong Hwang]

Created attachment 171960 [details]
Patch

[Dongseong Hwang]

(In reply to comment #4)
> Created an attachment (id=171960) [details]
> Patch

Rebased to upstream

[Dongseong Hwang]

Created attachment 171998 [details]
Patch

[Dongseong Hwang]

(In reply to comment #6)
> Created an attachment (id=171998) [details]
> Patch

Patch for EWS analysis after dependencies are removed

[EFL EWS Bot]

Comment on attachment 171998 [details]
Patch

Attachment 171998 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14687759

[Noam Rosenthal]

Comment on attachment 171998 [details]
Patch

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.

[Dongseong Hwang]

(In reply to comment #9)
> (From update of attachment 171998 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171998&action=review
> 

Thanks for your review!
We will review this bug again after we decide whether to parallelize TexMap or Coordinated Graphics.

> Recapping from IRC:
> 1. If the other implementation of CoordinatedSurface doesn't use IPC, why create CoordinatedSurface::encode/decode?

I will introduce new guards COORDINATED_GRAPHICS_UI_SIDE and COORDINATED_GRAPHICS_THREADED.
Both guards always are under COORDINATED_GRAPHICS.

You'er right, when COORDINATED_GRAPHICS_UI_SIDE is turned on, CoordinatedShareableSurface can be compiled only.
Using inheritance here in spite of only one implementation is that I don't want to put #if guard on CoordinatedSurface.h to support exclusive requirements of UI_SIDE and THREADED.
I want GraphicsContext being pure abstract class because of the similar reason.
BTW, I will remove CoordinatedShareableSurface and reimplement CoordinatedSurface by impl idiom. After that, it is more obvious we choose in a compile time.

> 2. Seems like the choice of implementations should be in compile time and not runtime.

As I mentioned above, we choose in a compile time.

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

I think making TextureMapperLayer run in a compositing thread is good, too. As you mentioned in IRC, we need to refactor that TextureMapperLayer uses TiledBackingStore, which has the ability to manage textures. I name it as TexMapTBS.
TexMapTBS will play a role like LayreTiler in Blackberry threaded compositor. TexMapTBS has the responsibility to copy a image on main thread to a texture on the compositing thread. Both main thread and compositing thread need to access TexMapTBS and this code will be complex. It is why LayerTiler code in blackberry compositor is complex.
I prefer Coordinated Graphics design to blackberry compositor design because it is obvious which code belongs to Web Process or UI Process in Coordinated Graphics. Blackberry compositor code mixes main thread part and compositing thread part together, especially in LayerTiler.
I've not confidence to make as succinct code as Coordinated Graphics in that I parallelize TextureMapperLayer using TexMapTBS but I'll try. We can discuss next week.

I put the comparison among three compositors in WebKit.

                                                         TexMap                 ChromiumCompositor           BlackBerry
Layer in the main thread :             CoordinatedGraphicsLayer        LayerChromium            LayerWebKitThread
Layer off the main thread :               TextureMapperLayer                  CCLayer              LayerCompositingThread
Layer manager in the main thread :   LayerTreeCoordinator            CCLayerTreeHost     WebPagePrivate && FrameLayers
Layer manager off the main thread :     LayerTreeRenderer         CCLayerTreeHostImpl          LayerRenderer
Tiles in the main thread :                          TBS                     TiledLayerChromium            LayerTiler
Tiles off the main thread :            CoordinatedBackingStore        CCTiledLayerImpl               LayerTiler

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

The purpose of flags indicates alpha or opaque. I don't want Coordinated Graphics to know ShareableBitmap::Flag, which is too far abstraction. If you don't like boolean, how about introducing new enum in CoordinatedSurface?

> > Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedSurface.h:61
> > +    virtual CoordinatedSurfaceID id() const = 0;
> 
> This is only relevant to IPC

TileID and LayerID as well as CoordinatedSurfaceID are needed to Threaded too. In detail, LayerTreeCoordinatedProxy in thread mode also caches CoordinatedSurfaces. When LayerTreeCoordinatedProxy receive TileUpdate message, LayerTreeCoordinatedProxy need to know CoordinatedSurfaceID to get CoordinatedSurface on which the updated tile draws.
 
> > 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?

No, we decide in a compile time. The method is only for debugging. I'll remove the method as reimplemeting CoordinatedSurface by impl idiom.

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

Yes, exactly.

[Dongseong Hwang]

Created attachment 175727 [details]
Patch

[Dongseong Hwang]

(In reply to comment #11)
> Created an attachment (id=175727) [details]
> Patch

This bug revives.

We need sort of surface abstraction that coordinated graphics will use in WebCore.
Currently coordinated graphics uses ShareableSurface, but after we move some classes in coordinated graphics (e.g. CoordinatedTile), we can not use ShareableSurface.

So this patch introduces CoordinatedSurface.

[Noam Rosenthal]

Comment on attachment 175727 [details]
Patch

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

Why not just move ShareableSurface to the CoordinatedGraphics folder, and have it inherit from CoordinatedSurface? This patch can be a lot smaller...

> Source/WebKit2/ChangeLog:17
> +        this patch introduces new class: CoordinatedSurface. After this patch,
> +        Coordinated Graphics draws something on CoordinatedSurface.

this patch introduces new class: CoordinatedSurface, to be used as a drawing surface for Coordinated Graphics.

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

bool alpha == boolean trap

[Dongseong Hwang]

(In reply to comment #13)
> (From update of attachment 175727 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=175727&action=review
> 
> Why not just move ShareableSurface to the CoordinatedGraphics folder, and have it inherit from CoordinatedSurface? This patch can be a lot smaller...

Because I could not imagine yet. That's good idea. I'll update :)

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

You're right! I'll change boolean to enum as following this guide line: http://doc.qt.digia.com/qq/qq13-apis.html#thebooleanparametertrap.

[Dongseong Hwang]

Created attachment 175852 [details]
Patch

[Noam Rosenthal]

Comment on attachment 175852 [details]
Patch

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

Much better, but requires some tweaking.

> Source/WebKit2/ChangeLog:25
> +        Currently, CoordinatedGraphicsLayer, CoordinatedTile and
> +        CoordinatedBackingStore directly use ShareableSurface. ShareableSurface was
> +        implemented to share an image between processes as well as placed in WK2.
> +        We want them to not depend on ShareableSurface to move them into WebCore. So
> +        this patch introduces new class: CoordinatedSurface, to be used as a
> +        drawing surface for Coordinated Graphics.
> +
> +        When we move some classes of Coordinated Graphics into WebCore,
> +        CoordinatedSurface.h will be moved too, but CoordinatedShareableSurface
> +        will be left in WK2.
> +
> +        We will introduce CoordinatedThreadSafeSurface later, which plays a role in
> +        sharing an surface between threads. Coordinated Graphics on WK1 will use
> +        CoordinatedThreadSafeSurface.

Can you focus here less about the whole story and more about what you're doing in the actual patch?
Something like,
Create a CoordinatedSurface class that can be the base class both for the current IPC-based ShareableSurface, renamed here to WebCoordinatedSurface, and to a future thread-based surface implementation.

> Source/WebKit2/ChangeLog:68
> +          This struct exists to pass CoordinatedSurface to UI Process. We can
> +          not pass the message including non-POD members between processes yet.

That's not accurate.

> Source/WebKit2/CMakeLists.txt:220
> +    Shared/CoordinatedGraphics/CoordinatedShareableSurface.cpp

Let's call this creature WebCoordinatedSurface.

> Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsArgumentCoders.cpp:33
> +#include "CoordinatedSurfaceInfo.h"

Instead of creating a new file for this, why don't we add at the end of WebCoordinatedSurface:
typedef RefPtr<WebCoordinatedSurface> WebCoordinatedSurfaceRef;

> Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedSurface.h:47
> +    enum Alpha {
> +        Opaque,
> +        SupportsAlpha
> +    };

Let's use flags like the other classes. Then we can add more flags later if we have to.

> Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedSurface.h:55
> +    static bool supportsAlpha(Alpha alpha) { return alpha == Opaque ? false : true; }
> +    static Alpha toAlpha(bool supportsAlpha) { return supportsAlpha ? SupportsAlpha : Opaque; }
> +
> +    virtual Alpha alpha() const = 0;

Are these really needed?
What about 
CoordinatedSurface::supportsAlpha() { return flags() & SupportsAlpha; }

> Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedSurfaceInfo.h:39
> +struct CoordinatedSurfaceInfo {
> +    RefPtr<CoordinatedSurface> coordinatedSurface;
> +};

This file is not needed, see above.

[Dongseong Hwang]

Comment on attachment 175852 [details]
Patch

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

>> Source/WebKit2/ChangeLog:25
>> +        CoordinatedThreadSafeSurface.
> 
> Can you focus here less about the whole story and more about what you're doing in the actual patch?
> Something like,
> Create a CoordinatedSurface class that can be the base class both for the current IPC-based ShareableSurface, renamed here to WebCoordinatedSurface, and to a future thread-based surface implementation.

Thank you for good advice!

>> Source/WebKit2/ChangeLog:68
>> +          not pass the message including non-POD members between processes yet.
> 
> That's not accurate.

I see.

>> Source/WebKit2/CMakeLists.txt:220
>> +    Shared/CoordinatedGraphics/CoordinatedShareableSurface.cpp
> 
> Let's call this creature WebCoordinatedSurface.

great suggestion!

>> Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsArgumentCoders.cpp:33
>> +#include "CoordinatedSurfaceInfo.h"
> 
> Instead of creating a new file for this, why don't we add at the end of WebCoordinatedSurface:
> typedef RefPtr<WebCoordinatedSurface> WebCoordinatedSurfaceRef;

Ok for not creating a new file, but typedef is impossible.

Currently we can use only struct or class for argument of message.
In the detail,
messages.py generates forward declarations in LayerTreeCoordinatorProxyMessages.h
#include "Arguments.h"
#include "MessageEncoder.h"
#include "MessageID.h"
#include "StringReference.h"
#include <wtf/Vector.h>

namespace WebCore {
    class CustomFilterProgramInfo;
    class FilterOperations;
    class GraphicsLayerAnimations;
    class IntSize;
    class GraphicsSurfaceToken;
    class Color;
    class IntRect;
    class IntPoint;
}

namespace WebKit {
    class WebLayerInfo;
    struct CoordinatedSurfaceRef;
    class SurfaceUpdateInfo;
}


struct SetCompositingLayerState : CoreIPC::Arguments2<uint32_t, const WebKit::WebLayerInfo&> {
    static const Kind messageID = SetCompositingLayerStateID;
    static CoreIPC::StringReference receiverName() { return messageReceiverName(); }
    static CoreIPC::StringReference name() { return CoreIPC::StringReference("SetCompositingLayerState"); }
    static const bool isSync = false;

    typedef CoreIPC::Arguments2<uint32_t, const WebKit::WebLayerInfo&> DecodeType;
    SetCompositingLayerState(uint32_t id, const WebKit::WebLayerInfo& layerInfo)
        : CoreIPC::Arguments2<uint32_t, const WebKit::WebLayerInfo&>(id, layerInfo)
    {
    }
};


There are only two options in messages.py: class or struct.
def struct_or_class(namespace, type):
    structs = frozenset([
        ...
        'WebCore::TransformOperation',
        'WebCore::TransformOperations',
        'WebKit::CoordinatedSurfaceRef',
        'WebKit::DictionaryPopupInfo',
        ...
    ])

    qualified_name = '%s::%s' % (namespace, type)
    if qualified_name in structs:
        return 'struct %s' % type

    return 'class %s' % type

so, we need struct to use as an argument of message, unfortunately.

>> Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedSurface.h:47
>> +    };
> 
> Let's use flags like the other classes. Then we can add more flags later if we have to.

absolutely! I did not know that kind of convention :)

>> Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedSurface.h:55
>> +    virtual Alpha alpha() const = 0;
> 
> Are these really needed?
> What about 
> CoordinatedSurface::supportsAlpha() { return flags() & SupportsAlpha; }

I see. I misunderstood your past request.

>> Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedSurfaceInfo.h:39
>> +};
> 
> This file is not needed, see above.

Yes

[Noam Rosenthal]

> > Instead of creating a new file for this, why don't we add at the end of WebCoordinatedSurface:
> > typedef RefPtr<WebCoordinatedSurface> WebCoordinatedSurfaceRef;
> 
> Ok for not creating a new file, but typedef is impossible.
Source/WebKit2/Scripts/webkit2/messages.py, look for special cases :)

[Dongseong Hwang]

Created attachment 177979 [details]
Patch

[Dongseong Hwang]

(In reply to comment #18)
> > > Instead of creating a new file for this, why don't we add at the end of WebCoordinatedSurface:
> > > typedef RefPtr<WebCoordinatedSurface> WebCoordinatedSurfaceRef;
> > 
> > Ok for not creating a new file, but typedef is impossible.
> Source/WebKit2/Scripts/webkit2/messages.py, look for special cases :)

We are the first to use type except for struct and class as an argument of messages. However, your instruction helps me to change messages.py to handle typedef!

[Noam Rosenthal]

Comment on attachment 177979 [details]
Patch

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

This changes the behavior implicitly in a way that the bitmap handle is alive for the lifetime of the surface. We should find a way to make this refactor without changing this behavior.

> Source/WebKit2/ChangeLog:36
> +        * Scripts/webkit2/messages.py:
> +        (forward_declarations_and_headers):
> +          This patch needs to use typedef as an argument of messages, so this patch changes
> +          forward_declarations_and_headers function to include headers for types in special_cases set,
> +          not to make forward declarations.
> +        (headers_for_type):
> +          CoordinatedSurfaceRef's header needs to be special cased to
> +          CoordinatedSurface.h.

Please review this in a separate bug report, mentioning this one as a use case.

> Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedSurface.h:58
> +    virtual const WebCore::IntSize& size() const = 0;

The const& thing is not needed... An IntSize is as expensive to copy as a pointer. Keep it the same as WebCore::Image::size

> Source/WebKit2/Shared/CoordinatedGraphics/WebCoordinatedSurface.h:118
> +    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;

This should be in DEBUG ifdef.

[Dongseong Hwang]

(In reply to comment #21)
> (From update of attachment 177979 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=177979&action=review

Thank you for review!

> This changes the behavior implicitly in a way that the bitmap handle is alive for the lifetime of the surface. We should find a way to make this refactor without changing this behavior.

Could you explain why bitmap handle has the same lifetime of the surface? I can destroy handle as soon as sending creation message to UI Process, but it also changes behavior. IMO, this change is acceptable.

> > Source/WebKit2/ChangeLog:36
> > +        * Scripts/webkit2/messages.py:
> > +        (forward_declarations_and_headers):
> > +          This patch needs to use typedef as an argument of messages, so this patch changes
> > +          forward_declarations_and_headers function to include headers for types in special_cases set,
> > +          not to make forward declarations.
> > +        (headers_for_type):
> > +          CoordinatedSurfaceRef's header needs to be special cased to
> > +          CoordinatedSurface.h.
> 
> Please review this in a separate bug report, mentioning this one as a use case.

I see. I'll make new patch with applying WebLayerID (for example)

> > Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedSurface.h:58
> > +    virtual const WebCore::IntSize& size() const = 0;
> 
> The const& thing is not needed... An IntSize is as expensive to copy as a pointer. Keep it the same as WebCore::Image::size

You'r right! I had copied and pasted without consiousness..

> > Source/WebKit2/Shared/CoordinatedGraphics/WebCoordinatedSurface.h:118
> > +    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;
> 
> This should be in DEBUG ifdef.

I see.

[Dongseong Hwang]

(In reply to comment #22)
> (In reply to comment #21)
> > (From update of attachment 177979 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=177979&action=review
> 
> Thank you for review!
> 
> > This changes the behavior implicitly in a way that the bitmap handle is alive for the lifetime of the surface. We should find a way to make this refactor without changing this behavior.
> 
> Could you explain why bitmap handle has the same lifetime of the surface? I can destroy handle as soon as sending creation message to UI Process, but it also changes behavior. IMO, this change is acceptable.
> 

Question is weird. I mean: Could you explain why don't you want bitmap handle to have the same lifetime of the surface?

[Noam Rosenthal]

(In reply to comment #23)
> Question is weird. I mean: Could you explain why don't you want bitmap handle to have the same lifetime of the surface?

Maintaining that handle might not be free, depending on the platform.
But the main issue is that this implicit behavior change shouldn't come in a class-hierarchy refactor patch like this one.

[Dongseong Hwang]

(In reply to comment #24)
> (In reply to comment #23)
> > Question is weird. I mean: Could you explain why don't you want bitmap handle to have the same lifetime of the surface?
> 
> Maintaining that handle might not be free, depending on the platform.
> But the main issue is that this implicit behavior change shouldn't come in a class-hierarchy refactor patch like this one.

Ok, firstly I 'll make just class-hierarchy refactor and in the second, make UpdateInfo not need to handle explicitly.

In the detail,
1. WebCoordinatedSurface's constructor makes the handle.
2. Send handle to UI Process
3. Destroy the handle as soon as sending handle to UI Process.

How about that?

[Dongseong Hwang]

Created attachment 178168 [details]
Patch

[Dongseong Hwang]

(In reply to comment #26)
> Created an attachment (id=178168) [details]
> Patch

I posted the patch changing messages.py in Bug 104339.
This patch focuses on only refactoring about class-hierarchy.

I'll post follow-up patch to remove WebCoordinatedSurface::Handler in Coordinated Graphics

[Dongseong Hwang]

(In reply to comment #27)
> (In reply to comment #26)
> > Created an attachment (id=178168) [details] [details]
> > Patch
> 
> I posted the patch changing messages.py in Bug 104339.

andersca disagrees about typedef in Bug 104339.
I can work around via creating something like struct CoordinatedSurfaceInfo.
How do you think about it.

[Noam Rosenthal]

(In reply to comment #28)
> (In reply to comment #27)
> > (In reply to comment #26)
> > > Created an attachment (id=178168) [details] [details] [details]
> > > Patch
> > 
> > I posted the patch changing messages.py in Bug 104339.
> 
> andersca disagrees about typedef in Bug 104339.
> I can work around via creating something like struct CoordinatedSurfaceInfo.
> How do you think about it.

Just encode/decode the handle and extract it like we did before.

[Dongseong Hwang]

Created attachment 178341 [details]
Patch

[Dongseong Hwang]

(In reply to comment #29)
> (In reply to comment #28)
> > (In reply to comment #27)
> > > (In reply to comment #26)
> > > > Created an attachment (id=178168) [details] [details] [details] [details]
> > > > Patch
> > > 
> > > I posted the patch changing messages.py in Bug 104339.
> > 
> > andersca disagrees about typedef in Bug 104339.
> > I can work around via creating something like struct CoordinatedSurfaceInfo.
> > How do you think about it.
> 
> Just encode/decode the handle and extract it like we did before.

Yes, I see.

[Dongseong Hwang]

(In reply to comment #30)
> Created an attachment (id=178341) [details]
> Patch

This patch is about only class hierarchy refactoring.
patch about removing handle will be posted in Bug 104347.

[Noam Rosenthal]

Comment on attachment 178341 [details]
Patch

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

I'm ok with the change, but the toFlags(...) thing is not an improvement IMO, and the signature of the size() function wasn't changed.

> Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedSurface.h:58
> +    virtual const WebCore::IntSize& size() const = 0;

Didn't we agree to make this 
virtual WebCore::IntSize size() const = 0?

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:743
> -    return m_coordinator->beginContentUpdate(size, contentsOpaque() ? 0 : ShareableBitmap::SupportsAlpha, atlas, offset);
> +    return m_coordinator->beginContentUpdate(size, CoordinatedSurface::toFlags(!contentsOpaque()), atlas, offset);

This toFlags change doesn't improve readability.

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/LayerTreeCoordinator.cpp:778
> -        if (atlas->flags() == flags) {
> +        if (atlas->supportsAlpha() == CoordinatedSurface::supportsAlpha(flags)) {

This doesn't improve readability.

[Dongseong Hwang]

Created attachment 178465 [details]
Patch

[Dongseong Hwang]

(In reply to comment #33)
> (From update of attachment 178341 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=178341&action=review
> 
> I'm ok with the change, but the toFlags(...) thing is not an improvement IMO, and the signature of the size() function wasn't changed.
> 
> > Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedSurface.h:58
> > +    virtual const WebCore::IntSize& size() const = 0;
> 
> Didn't we agree to make this 
> virtual WebCore::IntSize size() const = 0?

It's mistake. I missed.

> 
> > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:743
> > -    return m_coordinator->beginContentUpdate(size, contentsOpaque() ? 0 : ShareableBitmap::SupportsAlpha, atlas, offset);
> > +    return m_coordinator->beginContentUpdate(size, CoordinatedSurface::toFlags(!contentsOpaque()), atlas, offset);
> 
> This toFlags change doesn't improve readability.

Ok, I'll remove.

> 
> > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/LayerTreeCoordinator.cpp:778
> > -        if (atlas->flags() == flags) {
> > +        if (atlas->supportsAlpha() == CoordinatedSurface::supportsAlpha(flags)) {
> 
> This doesn't improve readability.

Ok, I'll remove.

[Dongseong Hwang]

Created attachment 178466 [details]
Patch

[Noam Rosenthal]

Comment on attachment 178466 [details]
Patch

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

> Source/WebKit2/ChangeLog:17
> +        Currently, CoordinatedGraphicsLayer, CoordinatedTile and
> +        CoordinatedBackingStore directly use ShareableSurface. ShareableSurface was
> +        implemented to share an image between processes as well as placed in WK2.
> +        We want them to not depend on ShareableSurface to move them into WebCore. So
> +        this patch introduces new class: CoordinatedSurface, to be used as a
> +        drawing surface for Coordinated Graphics.

Please remove this part; It needs some English correction and is not actually necessary :)

[Dongseong Hwang]

Created attachment 178472 [details]
Patch

[Dongseong Hwang]

(In reply to comment #37)
> (From update of attachment 178466 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=178466&action=review
> 
> > Source/WebKit2/ChangeLog:17
> > +        Currently, CoordinatedGraphicsLayer, CoordinatedTile and
> > +        CoordinatedBackingStore directly use ShareableSurface. ShareableSurface was
> > +        implemented to share an image between processes as well as placed in WK2.
> > +        We want them to not depend on ShareableSurface to move them into WebCore. So
> > +        this patch introduces new class: CoordinatedSurface, to be used as a
> > +        drawing surface for Coordinated Graphics.
> 
> Please remove this part; It needs some English correction and is not actually necessary :)

Thanks for review! I removed :)

[WebKit Review Bot]

Comment on attachment 178472 [details]
Patch

Clearing flags on attachment: 178472

Committed r137117: <http://trac.webkit.org/changeset/137117>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Mikhail Pozdnyakov]

Comment on attachment 178472 [details]
Patch

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

> Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedSurface.h:49
> +    bool supportsAlpha() { return flags() & SupportsAlpha; }

Looks strange that this method is not const

> Source/WebKit2/Shared/CoordinatedGraphics/WebCoordinatedSurface.cpp:44
> +    encoder.encode(m_size);

encoder << ?

> Source/WebKit2/Shared/CoordinatedGraphics/WebCoordinatedSurface.cpp:208
> +        texture->updateContents(image.get(), target, IntPoint::zero(), BitmapTexture::UpdateCanModifyOriginalImageData);

interesting why we don't return here

[Dongseong Hwang]

Reopening to attach new patch.

[Dongseong Hwang]

Created attachment 178487 [details]
Patch : It is a follow-up patch of r137117.

[Dongseong Hwang]

(In reply to comment #42)
> (From update of attachment 178472 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=178472&action=review
> 
> > Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedSurface.h:49
> > +    bool supportsAlpha() { return flags() & SupportsAlpha; }
> 
> Looks strange that this method is not const

Thank you for finding. I missed.

> > Source/WebKit2/Shared/CoordinatedGraphics/WebCoordinatedSurface.cpp:44
> > +    encoder.encode(m_size);
> 
> encoder << ?

I'll change. I just copied this code from ShareableSurface.

> > Source/WebKit2/Shared/CoordinatedGraphics/WebCoordinatedSurface.cpp:208
> > +        texture->updateContents(image.get(), target, IntPoint::zero(), BitmapTexture::UpdateCanModifyOriginalImageData);
> 
> interesting why we don't return here

I just copied, too. I'll fix.

[Dongseong Hwang]

Created attachment 178491 [details]
Patch : WebCoordinatedSurface::copyToTexture should return early if the backend is GraphicsSurface.

[Kenneth Rohde Christiansen]

Comment on attachment 178491 [details]
Patch : WebCoordinatedSurface::copyToTexture should return early if the backend is GraphicsSurface.

I have to trust you on this

[Dongseong Hwang]

(In reply to comment #47)
> (From update of attachment 178491 [details])
> I have to trust you on this

I think you can trust me on this patch at least. :)

[WebKit Review Bot]

Comment on attachment 178491 [details]
Patch : WebCoordinatedSurface::copyToTexture should return early if the backend is GraphicsSurface.

Clearing flags on attachment: 178491

Committed r137125: <http://trac.webkit.org/changeset/137125>

[WebKit Review Bot]

Comment on attachment 178487 [details]
Patch : It is a follow-up patch of r137117.

Clearing flags on attachment: 178487

Committed r137126: <http://trac.webkit.org/changeset/137126>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.