Bug 108294 – Coordinated Graphics: CoordinatedGraphicsLayer makes CoordinatedGraphicsScene perform via CoordinatedGraphicsOperation.

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Dongseong Hwang]

CoordinatedLayerTreeHostProxy has so many IPC messages (e.g. SyncCanvas and CreateTile), and there are function chain from CoordinatedGraphicsLayer to LayerTreeRenderer (4 classes).
If we want to add new function, we need to add boilerplate code into 4 classes.

Now CoordinatedLayerTreeHost has only one IPC message: CommitCooridnatedGraphicsOperations.
CoordinatedGraphicsLayer makes TextureMapperScene run as follows
1. CoordinatedGraphicsLayer makes CooridnatedGraphicsOperation (e.g. CreateTile).
2. CoordinatedLayerTreeHost stores all operations.
3. CoordinatedLayerTreeHost sends all operations to CoordinatedLayerTreeHostProxy at the time to flush via CommitCooridnatedGraphicsOperations message.
4. TextureMapperScene executes all operations.

There is one big behavior change. All operations (e.g. createLayer, syncCanvas, etc.) are performed at the same time, when TextureMapperScene::commitCooridnatedGraphicsOperations is called.

[Dongseong Hwang]

Created attachment 186001 [details]
WIP: I'll change to use Data for simple encoding

[Dongseong Hwang]

Created attachment 186006 [details]
WIP: use Data to remove switch statement in encoding.

IMO it is impossible to remove switch statement.
SimpleArgumentCoder just copy binary data, so in this example, the heap memory of "Vector<CoordinatedLayerID> layerIDs" cannot be encoded.

Moreover, CoordinatedGraphicsScene must derefer one level more:
from operation->layerIDs to operation->data->layerIDs.

I don't have good idea to remove switch statement yet.

[Noam Rosenthal]

(In reply to comment #2)
> Created an attachment (id=186006) [details]
> WIP: use Data to remove switch statement in encoding.
> 
> IMO it is impossible to remove switch statement.
> SimpleArgumentCoder just copy binary data, so in this example, the heap memory of "Vector<CoordinatedLayerID> layerIDs" cannot be encoded.
> 
> Moreover, CoordinatedGraphicsScene must derefer one level more:
> from operation->layerIDs to operation->data->layerIDs.
> 
> I don't have good idea to remove switch statement yet.
We can't remove it completely, but we can use a single case: for all the operations that have a "raw" data type.

[Dongseong Hwang]

Created attachment 186014 [details]
Patch

[Dongseong Hwang]

Created attachment 186016 [details]
Patch

[Dongseong Hwang]

(In reply to comment #3)
> (In reply to comment #2)
> > Created an attachment (id=186006) [details] [details]
> > Moreover, CoordinatedGraphicsScene must derefer one level more:
> > from operation->layerIDs to operation->data->layerIDs.
> > 
> > I don't have good idea to remove switch statement yet.
> We can't remove it completely, but we can use a single case: for all the operations that have a "raw" data type.

yes, I see.
For example, the same case can encode SetRootLayerType, SetLayerStateType,  CreateTileType,  UpdateTileType, and  RemoveTileType because all are just POD.
But CoordinatedGraphicsScene need to change from "static_cast<const CoordinatedGraphicsOperation::SetRootLayer*>(operation)->layerID" to "static_cast<const CoordinatedGraphicsOperation::SetRootLayer*>(operation)->data->layerID"

Do you think we should go to Data?
If so, I'll apply some Operations that have just POD data.

[Dongseong Hwang]

Comment on attachment 186016 [details]
Patch

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

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsOperation.h:98
> +        : layerIDs(ids)

I'll indent

[Noam Rosenthal]

Comment on attachment 186016 [details]
Patch

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

Have some comments, but I think it's a good direction. More input from others welcome...

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.h:48
> +class CoordinatedGraphicsLayerClient : public CoordinatedGraphicsOperation::Queue {

I wouldn't use inheritance here.
Instead, CoordinatedGraphicsLayerClient should have a function that returns a queue.

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsOperation.h:59
> +        CreateLayersType,
> +        DeleteLayersType,
> +        SetRootLayerType,
> +        SetLayerStateType,
> +        SetLayerChildrenType,
> +        CreateTileType,
> +        UpdateTileType,
> +        RemoveTileType,
> +        CreateUpdateAtlasType,
> +        RemoveUpdateAtlasType,

...
Let's use caps here, CREATE_LAYERS etc., like FilterOperation and TransformOperation. Then you also don't need the word Type everywhere.

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsOperation.h:83
> +        NoneType

I don't see the point of NoneType.

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsOperation.h:86
> +    class Operation : public RefCounted<Operation> {

I don't see the point of this inner class... This virtual class can be part of CoordinatedGraphicsOperation, and the subclasses can be inner classes.
Also, it has to be ThreadSafeRefCounted.

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.cpp:614
> +void CoordinatedLayerTreeHost::enqueueCoordinatedGraphicsOperation(PassRefPtr<CoordinatedGraphicsOperation::Operation> operation)

I think this should be divided to two functions. One called willEnqueueCoordinatedGraphicsOperation, that handles your switch statement and is called from this function. This function should only call willEnqueue, and then append to the vector.

[Mikhail Pozdnyakov]

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

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsOperation.h:83
> +        NoneType

Do we really need it?

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsOperation.h:87
> +    public:

virtual destructor?

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsOperation.h:91
> +    class CreateLayers : public Operation {

How about template<OperationType Type> OperationBase { CoordinatedGraphicsOperation::OperationType getOperationType() const {return Type} }; ?
That would let you not to override 'getOperationType' in every implementation but rather inherit from: OperationBase<NeededType>

Also we could think of adding another template parameter for the type of the contained data, and that I believe could significantly reduce the size of this patch :)

[Caio Marcelo de Oliveira Filho]

Comment on attachment 186016 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        If we want to add a new message, we need to add boilerplate code into 4 classes.

Suggestion: I would mention in the ChangeLog what need to be changed to add a new operation now: create new operation subclass, code to handle in executeCoordinatedGraphicsOperation(), encode/decode and optionally special treatment in enqueue.

>> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsOperation.h:86
>> +    class Operation : public RefCounted<Operation> {
> 
> I don't see the point of this inner class... This virtual class can be part of CoordinatedGraphicsOperation, and the subclasses can be inner classes.
> Also, it has to be ThreadSafeRefCounted.

Agreed that you should merge Operation into CoordinatedGraphicsOperation.

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsOperations.h:44
> +    const CoordinatedGraphicsOperation::Operation* at(size_t index) const { return index < m_operations.size() ? m_operations.at(index).get() : 0; }

Do we need this bound checking version of at()?

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsScene.cpp:574
> +void CoordinatedGraphicsScene::executeCoordinatedGraphicsOperation(const CoordinatedGraphicsOperation::Operation* operation)

Did you consider the approach of making a "virtual void execute(CoordinatedGraphicsScene*)" virtual function in Operation? Then you could simply call it here and avoid the switch. The piece of code for each operation would be in the operations themselves, with no static_cast to clutter the code.

> Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsArgumentCoders.cpp:867
> +            encoder << static_cast<const CoordinatedGraphicsOperation::CreateTile*>(operation)->layerID;
> +            encoder << static_cast<const CoordinatedGraphicsOperation::CreateTile*>(operation)->tileID;
> +            encoder << static_cast<const CoordinatedGraphicsOperation::CreateTile*>(operation)->tileRect;
> +            encoder << static_cast<const CoordinatedGraphicsOperation::CreateTile*>(operation)->updateInfo;

For POD types you could use SimpleArgumentCoder. In this case those lines could be replaced by SimpleArgumentCoder<CoordinatedGraphicsOperation::CreateTile>::encode(encoder, operation). Same trick can be used for decoder.

[Noam Rosenthal]

> 
> Did you consider the approach of making a "virtual void execute(CoordinatedGraphicsScene*)" virtual function in Operation? Then you could simply call it here and avoid the switch. The piece of code for each operation would be in the operations themselves, with no static_cast to clutter the code.

This was in the original version... Maybe it is a better idea than the giant switch statement.

> For POD types you could use SimpleArgumentCoder. In this case those lines could be replaced by SimpleArgumentCoder<CoordinatedGraphicsOperation::CreateTile>::encode(encoder, operation). Same trick can be used for decoder.
We would need to have a Data struct for the data, since we don't want to do this for the RefCounted header.

[Caio Marcelo de Oliveira Filho]

(In reply to comment #11)
> > For POD types you could use SimpleArgumentCoder. In this case those lines could be replaced by SimpleArgumentCoder<CoordinatedGraphicsOperation::CreateTile>::encode(encoder, operation). Same trick can be used for decoder.
> We would need to have a Data struct for the data, since we don't want to do this for the RefCounted header.

Hmm. Right.

[Dongseong Hwang]

Thank Noam,  Mikhail and Caio for your review!

(In reply to comment #8)
> (From update of attachment 186016 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186016&action=review
> > Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsOperation.h:59
> > +        CreateUpdateAtlasType,
> > +        RemoveUpdateAtlasType,
> 
> ...
> Let's use caps here, CREATE_LAYERS etc., like FilterOperation and TransformOperation. Then you also don't need the word Type everywhere.

Unfortunately, CREATE_LAYERS is not allowed by WebKit Coding Style : http://www.webkit.org/coding/coding-style.html
"12. Enum members should use InterCaps with an initial capital letter."
 
> > Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsOperation.h:86
> > +    class Operation : public RefCounted<Operation> {
> 
> I don't see the point of this inner class... This virtual class can be part of CoordinatedGraphicsOperation, and the subclasses can be inner classes.
> Also, it has to be ThreadSafeRefCounted.

That's a good idea, but c++ does not support to inherit outer class from inner class as follows. It is because when compiling the inner class, the outer is not complete type yet.
class CGBase {
    virtual void foo(){}
    class Inner : public CGBase {
        virtual void foo() {}
    };
};

We have two requirements
1. we want to call like this : CoordinatedGraphicsOperation::CreateLayers::create
2. Single vector keeps all operations, so we need inheritance.

So I choose this code.
class CoordinatedGraphicsOperation {
     class OperationBase { ... }
     class CreateLayers : public OperationBase { ... }
};

We can go with nested namespace like this. But it is not common in WebKit.
namespace CoordinatedGraphicsOperation {
     class OperationBase { ... }
     class CreateLayers : public OperationBase { ... }
}

(In reply to comment #9)
> View in context: https://bugs.webkit.org/attachment.cgi?id=186016&action=review
> > Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsOperation.h:91
> > +    class CreateLayers : public Operation {
> 
> How about template<OperationType Type> OperationBase { CoordinatedGraphicsOperation::OperationType getOperationType() const {return Type} }; ?
> That would let you not to override 'getOperationType' in every implementation but rather inherit from: OperationBase<NeededType>
> 
> Also we could think of adding another template parameter for the type of the contained data, and that I believe could significantly reduce the size of this patch :)

Great! Thank you.


(In reply to comment #10)
> (From update of attachment 186016 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186016&action=review
> 
> > Source/WebCore/ChangeLog:11
> > +        If we want to add a new message, we need to add boilerplate code into 4 classes.
> 
> Suggestion: I would mention in the ChangeLog what need to be changed to add a new operation now: create new operation subclass, code to handle in executeCoordinatedGraphicsOperation(), encode/decode and optionally special treatment in enqueue.

Yes, I'll do.

(In reply to comment #11)
> > 
> > Did you consider the approach of making a "virtual void execute(CoordinatedGraphicsScene*)" virtual function in Operation? Then you could simply call it here and avoid the switch. The piece of code for each operation would be in the operations themselves, with no static_cast to clutter the code.
> 
> This was in the original version... Maybe it is a better idea than the giant switch statement.

Ok, I'll go to original 'execute' version.

> > For POD types you could use SimpleArgumentCoder. In this case those lines could be replaced by SimpleArgumentCoder<CoordinatedGraphicsOperation::CreateTile>::encode(encoder, operation). Same trick can be used for decoder.
> We would need to have a Data struct for the data, since we don't want to do this for the RefCounted header.

I also want to use SimpleArgumentCoder like your suggestion.
So I'll add Data struct to simple data operations like SetLayerState.

[Dongseong Hwang]

(In reply to comment #9)
> View in context: https://bugs.webkit.org/attachment.cgi?id=186016&action=review
> > Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsOperation.h:91
> > +    class CreateLayers : public Operation {
> 
> How about template<OperationType Type> OperationBase { CoordinatedGraphicsOperation::OperationType getOperationType() const {return Type} }; ?
> That would let you not to override 'getOperationType' in every implementation but rather inherit from: OperationBase<NeededType>
> 
> Also we could think of adding another template parameter for the type of the contained data, and that I believe could significantly reduce the size of this patch :)

To apply your suggestion, we need two level inheritance, because we need to have two base classes for different purpose respectively.
1. Single Vector can store all operation.
2. Base class should know Type info.

So we can not make it by one inheritance.

enum CGEnum {
    AAA,
    BBB,
};
template<CGEnum T> class CGBase {
    CGEnum type() { return T; }
};
class CGImpl : public CGBase<AAA> {
    void foo() {}
};
class CGBases {
    Vector<CGBase<AAA> > bases; // __This vector cannot keep CGBase<BBB>.__
};

We need two level inheritance. I'm not sure that it is readable. How about your opinion?
enum CGEnum {
    AAA,
    BBB,
};
class CGBase {
    virtual CGEnum type() = 0;
};
template<CGEnum T> class CGBaseTemplete : public CGBase {
    virtual CGEnum type() { return T; }

};
class CGImpl : public CGBaseTemplete<AAA> {
    void foo() {}
    int num;
};
class CGBaseVector {
    Vector<CGBase*> bases;
    void useCase()
    {
        static_cast<CGImpl*>(bases[0])->num; // __Is it safe?__
    }
};

[Dongseong Hwang]

Created attachment 186317 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 186317 [details]
Patch

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

> Source/WebCore/ChangeLog:15
> +        Now CoordinatedLayerTreeHost has only one IPC message: CommitCooridnatedGraphicsOperations.
> +        CoordinatedGraphicsLayer makes CoordinatedGraphicsScene run as follows:
> +        1. CoordinatedGraphicsLayer makes a CooridnatedGraphicsOperation (e.g. CreateTile).

Spelling issues here Cooridnated*

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:816
> +    m_coordinator->operationQueue()->enqueueCoordinatedGraphicsOperation(CoordinatedGraphicsOperation::RemoveTile::create(id(), tileID));

wouldnt operationQueue->encodeOperation not be sufficient?

[Dongseong Hwang]

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

> > For POD types you could use SimpleArgumentCoder. In this case those lines could be replaced by SimpleArgumentCoder<CoordinatedGraphicsOperation::CreateTile>::encode(encoder, operation). Same trick can be used for decoder.
> We would need to have a Data struct for the data, since we don't want to do this for the RefCounted header.

I made some trials, but all trials made CoordinatedGraphicsOperation.h more complex to remove some case statement in ArgumentCoder<CoordinatedGraphicsOperations>::encode().
I could not find good solution for this. But I open your suggestion! :)

[Dongseong Hwang]

(In reply to comment #16)
> (From update of attachment 186317 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186317&action=review
> Spelling issues here Cooridnated*

Oops. Thank you!

> > Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:816
> > +    m_coordinator->operationQueue()->enqueueCoordinatedGraphicsOperation(CoordinatedGraphicsOperation::RemoveTile::create(id(), tileID));
> 
> wouldnt operationQueue->encodeOperation not be sufficient?

I think your suggestion is sufficient. I'll change.

[Dongseong Hwang]

Created attachment 186318 [details]
Patch

[Mikhail Pozdnyakov]

Comment on attachment 186317 [details]
Patch

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

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsOperation.h:105
> +        explicit CreateLayers(const Vector<CoordinatedLayerID>& ids)

shouldn't it be private?

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsOperation.h:108
> +        virtual ~CreateLayers() { }

since the class is final, empty destructor can be omitted.

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsOperation.h:518
> +        virtual void enqueueCoordinatedGraphicsOperation(PassRefPtr<CoordinatedGraphicsOperation::OperationBase>) = 0;

virtual destructor is required.

[Mikhail Pozdnyakov]

> We need two level inheritance. I'm not sure that it is readable. How about your opinion?

Sure we need, but I consider it to be much better than copy/pasting same function to each class.

[Dongseong Hwang]

Created attachment 186322 [details]
Patch

[Dongseong Hwang]

(In reply to comment #20)
> (From update of attachment 186317 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186317&action=review
> shouldn't it be private?
exactly. Done.

> > Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsOperation.h:108
> > +        virtual ~CreateLayers() { }
> 
> since the class is final, empty destructor can be omitted.
> virtual destructor is required.

Thank you. Done.

> > We need two level inheritance. I'm not sure that it is readable. How about your opinion?
> Sure we need, but I consider it to be much better than copy/pasting same function to each class.

I'm sure now :)

[Dongseong Hwang]

Created attachment 186323 [details]
Patch

[Mikhail Pozdnyakov]

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

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsOperation.cpp:36
> +{

only one line.. maybe it's worth defining it inside the class declaration??

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsOperation.cpp:41
> +{

ditto, and same for the rest

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsOperation.h:109
> +        virtual void execute(CoordinatedGraphicsScene*) OVERRIDE;

seems this method can be const, as it just invokes some CoordinatedGraphicsScene method inside.

[Dongseong Hwang]

(In reply to comment #25)
> View in context: https://bugs.webkit.org/attachment.cgi?id=186318&action=review

Thank you for helping me improve code!

> > Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsOperation.cpp:36
> > +{
> 
> only one line.. maybe it's worth defining it inside the class declaration??
> ditto, and same for the rest

I decide to create cpp file for small two reasons.
1. I don't want CoordinatedGraphicsOperation.h to include CoordinatedGraphicsScene.h for build performance. Many files include CoordinatedGraphicsOperation.h.
2. I want to get together execute() implementations to see quickly what each CoordinatedGraphicsOperation performs using CoordinatedGraphicsScene.

> > Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsOperation.h:109
> > +        virtual void execute(CoordinatedGraphicsScene*) OVERRIDE;
> 
> seems this method can be const, as it just invokes some CoordinatedGraphicsScene method inside.

yes, I think const is possible, but I think it is more natural without const.
1. Other task classes implement something like execute() without const. for instance, FunctionWrapper::operator()() and FileThreadTask1::performTask().
2. CoordinatedGraphicsOperation::execute() will call any methods of CoordinatedGraphicsScene, so execute() with const is a bit weird.

Above is just my opinion. If your opinion is different, please let me know! :)

[Rafael Brandao]

Comment on attachment 186318 [details]
Patch

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

> Source/WebCore/ChangeLog:23
> +        CoordinatedGraphicsScene::commitCoordinatedGraphicsOperations is called.

This change is a huge win! It will avoid a lot of tricky bugs we've found in the past. :-)

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedImageBacking.cpp:42
> +PassRefPtr<CoordinatedImageBacking> CoordinatedImageBacking::create(CoordinatedGraphicsOperation::Queue* queue, PassRefPtr<Image> image)

Is it ok that we're keeping around a raw pointer of the queue? Is it possible that we run into enqueue function with a dangling pointer?

> Source/WebCore/platform/graphics/texmap/coordinated/UpdateAtlas.cpp:33
> +UpdateAtlas::UpdateAtlas(CoordinatedGraphicsOperation::Queue* queue, int dimension, CoordinatedSurface::Flags flags)

Same concern as above.

[Mikhail Pozdnyakov]

(In reply to comment #26)
> (In reply to comment #25)
> > View in context: https://bugs.webkit.org/attachment.cgi?id=186318&action=review
> 
> Thank you for helping me improve code!
> 
> > > Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsOperation.cpp:36
> > > +{
> > 
> > only one line.. maybe it's worth defining it inside the class declaration??
> > ditto, and same for the rest
> 
> I decide to create cpp file for small two reasons.
> 1. I don't want CoordinatedGraphicsOperation.h to include CoordinatedGraphicsScene.h for build performance. Many files include CoordinatedGraphicsOperation.h.
> 2. I want to get together execute() implementations to see quickly what each CoordinatedGraphicsOperation performs using CoordinatedGraphicsScene.
> 
> > > Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsOperation.h:109
> > > +        virtual void execute(CoordinatedGraphicsScene*) OVERRIDE;
> > 
> > seems this method can be const, as it just invokes some CoordinatedGraphicsScene method inside.
> 
> yes, I think const is possible, but I think it is more natural without const.
> 1. Other task classes implement something like execute() without const. for instance, FunctionWrapper::operator()() and FileThreadTask1::performTask().
> 2. CoordinatedGraphicsOperation::execute() will call any methods of CoordinatedGraphicsScene, so execute() with const is a bit weird.
> 
> Above is just my opinion. If your opinion is different, please let me know! :)
I agree with your points, think we can keep it as it is.

[Dongseong Hwang]

(In reply to comment #27)
> (From update of attachment 186318 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186318&action=review
> 
> > Source/WebCore/ChangeLog:23
> > +        CoordinatedGraphicsScene::commitCoordinatedGraphicsOperations is called.
> 
> This change is a huge win! It will avoid a lot of tricky bugs we've found in the past. :-)

I hope. thank you! :)

> > Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedImageBacking.cpp:42
> > +PassRefPtr<CoordinatedImageBacking> CoordinatedImageBacking::create(CoordinatedGraphicsOperation::Queue* queue, PassRefPtr<Image> image)
> 
> Is it ok that we're keeping around a raw pointer of the queue? Is it possible that we run into enqueue function with a dangling pointer?

Currently CoordinatedGraphicsOperation::Queue is CoordinatedLayerTreeHost which holds CoordinatedImageBacking and UpdateAtlas as a OwnPtr, so queue can not be a dangling pointer.

(In reply to comment #28)
> > Above is just my opinion. If your opinion is different, please let me know! :)
> I agree with your points, think we can keep it as it is.

Thank you for good review.

[Dongseong Hwang]

As we discussed in IRC, we will remove all encode/decode code in CoordinatedGraphicsArgumentCoder, and then we will encode/decode CoordinatedGraphicsOperations in our own classes: CoordinatedGraphicsEncoder, CoordinatedGraphicsDecoder.
I explain how we go. If you have good idea, please let me know! :)

Client code will be changed as follows:
@@ -289,7 +289,9 @@ bool CoordinatedLayerTreeHost::flushPendingLayerChanges()
         IntRect coveredRect = toCoordinatedGraphicsLayer(m_nonCompositedContentLayer.get())->coverRect();
-        m_webPage->send(Messages::CoordinatedLayerTreeHostProxy::CommitCoordinatedGraphicsOperations(m_operations, m_visibleContentsRect.location(), contentsSize, coveredRect));
+        CoordinatedGraphicsEncoder encoder;
+        SharedBuffer buffer = encoder.encode(m_operations);
+        m_webPage->send(Messages::CoordinatedLayerTreeHostProxy::CommitCoordinatedGraphicsOperations(buffer, m_visibleContentsRect.location(), contentsSize, coveredRect));
         m_operations.clear();
         m_waitingForUIProcess = true;

@@ -54,8 +54,10 @@ void CoordinatedLayerTreeHostProxy::dispatchUpdate(const Function<void()>& funct
-void CoordinatedLayerTreeHostProxy::commitCoordinatedGraphicsOperations(const CoordinatedGraphicsOperations& operations, const FloatPoint& scrollPosition, const IntSize& contentsSize, const IntRect& coveredRect)
+void CoordinatedLayerTreeHostProxy::commitCoordinatedGraphicsOperations(const SharedBuffer& buffer, const FloatPoint& scrollPosition, const IntSize& contentsSize, const IntRect& coveredRect)
 {
+    CoordinatedGraphicsDecoder decode;
+    CoordinatedGraphicsOperations operations = decoder.decode(buffer);
     dispatchUpdate(bind(&CoordinatedGraphicsScene::commitCoordinatedGraphicsOperations, m_scene.get(), operations, scrollPosition));
     updateViewport();

Currently, CoordinatedGraphicsArgumentCoder has more than 1000 line code, and most of all deal with filters, animations and surface as well as operatons.
If CoordinatedGraphics[De|En]coder deals with encode/decode, CoordinatedGraphicsArgumentCoder no longer needs to encode/decode above stuffs.

CoordinatedGraphicsEncoder will extends WTF::Encoder as follows:
CoordinatedGraphicsEncoder : public WTF::Encoder { encodeAnimation() encodeFilter() encodeSurface() }

However, I'm not sure that inheritance of WTF::Encoder is necessary. how do you think?

CoordinatedGraphicsEncoder::encode serializes CoordinatedGraphicsOperations to binary data, while CoordinatedGraphicsDecoder::decode creates CoordinatedGraphicsOperations from binary data.
encoding/decoding  filters or animations is the implementation detail of CoordinatedGraphics[De|En]coder.

Now, could you feedback me? :D

Gwangyoon Hwang (ryumiel) will contribute to this encode/decode work, because I am going to vacation until next Tuesday. lol
So if you don't mind, I wish this patch is reviewed and ryumiel will work based on upstream webkit.

[Noam Rosenthal]

Comment on attachment 186323 [details]
Patch

As discussed on IRC, waiting for a new patch that uses templates instead of a lot of broilerplate code.

[Balazs Kelemen]

Comment on attachment 186323 [details]
Patch

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

> Source/WebKit2/UIProcess/CoordinatedGraphics/CoordinatedLayerTreeHostProxy.cpp:58
> -void CoordinatedLayerTreeHostProxy::didRenderFrame(const IntSize& contentsSize, const IntRect& coveredRect)
> +void CoordinatedLayerTreeHostProxy::commitCoordinatedGraphicsOperations(const CoordinatedGraphicsOperations& operations, const WebCore::IntSize& contentsSize, const WebCore::IntRect& coveredRect)
>  {

I would keep the name didRenderFrame, it is more straightforward to me.

[Dongseong Hwang]

(In reply to comment #32)
> (From update of attachment 186323 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186323&action=review
> 
> > Source/WebKit2/UIProcess/CoordinatedGraphics/CoordinatedLayerTreeHostProxy.cpp:58
> > -void CoordinatedLayerTreeHostProxy::didRenderFrame(const IntSize& contentsSize, const IntRect& coveredRect)
> > +void CoordinatedLayerTreeHostProxy::commitCoordinatedGraphicsOperations(const CoordinatedGraphicsOperations& operations, const WebCore::IntSize& contentsSize, const WebCore::IntRect& coveredRect)
> >  {
> 
> I would keep the name didRenderFrame, it is more straightforward to me.

Thank you for suggestion. How do you think, noam? :) commitCoordinatedGraphicsOperations was suggested by noam because there are no arguments like IntSize and IntRect except for operations when we discussed.

btw, ryumiel are doing best to complete this bug now.

[Gwang Yoon Hwang]

Created attachment 188273 [details]
WIP. Just for discussion

What I try to do is removing CoordiantedGraphicsArgumentCoder things from WebKit2.
CoordinatedGraphicsOperation[De|En]coder takes care about that.

And commitCoordinatedGraphicsOperations uses only CoreIPC::DataReference. 

Noam, could you give some feedback?
I think I need to check basic design before digging deeper.

TODO.
1. Implement unimplemented
2. Debug encoder/decoder
3. Provide "pretty" encode/decode interface in CoordinatedSurface
4. Find a way to remove duplicated implementations with CoreIPC

[Gwang Yoon Hwang]

(In reply to comment #34)
> Created an attachment (id=188273) [details]
> WIP. Just for discussion
> 
> What I try to do is removing CoordiantedGraphicsArgumentCoder things from WebKit2.
> CoordinatedGraphicsOperation[De|En]coder takes care about that.
> 
> And commitCoordinatedGraphicsOperations uses only CoreIPC::DataReference. 
> 
> Noam, could you give some feedback?
> I think I need to check basic design before digging deeper.
> 
> TODO.
> 1. Implement unimplemented
> 2. Debug encoder/decoder
> 3. Provide "pretty" encode/decode interface in CoordinatedSurface
> 4. Find a way to remove duplicated implementations with CoreIPC

Due to the way implemented CoordinatedGraphicsOperationBase, we cannot use plain WTF::encoder/decoder. (It doesn't provide templates)

I'll modify CoordinatedGraphicsOperation[En/De]coder to use WTF::encoder to remove duplication.

[Gwang Yoon Hwang]

Created attachment 189296 [details]
WIP. discussion for APIs for CoordinatedGraphicsOperation

I purpose several changes in this patch.

1.CoordinatedGraphicsOperation should be encoded right after the operation
is created. Because SharedableSurface::Handle is non copyable.

2. CoordinatedGraphicsScene::TileUpdate could be removed, SurfaceUpdateInfo
can be used instead of it. 

3. By implement overrided execute method in CoordinatedGraphicsOperation[0-5],
we can remove annoying codes in CoordinatedGraphicsOperation.cpp.

To support this, CoordinatedGraphicsOperation::[Operation] should be
matched with CoordinatedGraphicsScene::[method]. We need to modify types
of CoordinatedGraphicsOperation::UpdateImageBacking, CreateCanvas,
CreateUpdateAtlas, and CreateTile.


I need jobs to do before complete this bug.

1. CoordinatedSurface and GraphicsSurface should provide encode/decode
methods.

2. After discussion with noamr in IRC, we agreed about current code of
CoordinatedGraphicsArgumentCoders.cpp (CoordinatedGraphicsOperationCoders.cpp in this patch)
is a little bit messy,because it has 600 lines of code cares about FilterOperations,
TransformOperations, and TimingFunction.

To remove bunch of codes in CoordinatedGraphicsArgumentCoders, which is
needed to [en|de]code GraphicsLayerAnimation. We need to add [en|de]code
method to these classes.

This mess will be clean up in seperated bug.

[Balazs Kelemen]

> > Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.h:48
> > +class CoordinatedGraphicsLayerClient : public CoordinatedGraphicsOperation::Queue {
> 
> I wouldn't use inheritance here.
> Instead, CoordinatedGraphicsLayerClient should have a function that returns a queue.

I have a selfish request about this. Could we probably keep the interface of GraphicsLayerClient as is and make the queue an implementation deal, i.e. CoordintedLayerTreeHost would push the items for it's own? To achieve the goal of bug 109579 I need to be able to merge operations from more than one flush, so I need to preprocess them before sending (i.e. if there is two update for the same tile, I should only keep the second). I could possibly merge them just before sending the message but it would be more straighforward to do such filtering incrementally when an operation is enqueued. Actually we already have some cross dependencies across operations, that are handled in willEnqueue in the patch.

Another observation: I think we should keep RequestAnimationFrame to be a separate message because it should force a repaint in the UI immediately even if nothing triggers a flush in the web process.

[Noam Rosenthal]

> 2. After discussion with noamr in IRC, we agreed about current code of
> CoordinatedGraphicsArgumentCoders.cpp (CoordinatedGraphicsOperationCoders.cpp in this patch)
> is a little bit messy,because it has 600 lines of code cares about FilterOperations,
> TransformOperations, and TimingFunction.
> 
> To remove bunch of codes in CoordinatedGraphicsArgumentCoders, which is
> needed to [en|de]code GraphicsLayerAnimation. We need to add [en|de]code
> method to these classes.
> 
> This mess will be clean up in seperated bug.
I talked to andersca on IRC about this, and he asked that we wait before moving stuff to WTF::encoder, as he would like to make changes to the encoding/decoding infrastructure first.

[Gwang Yoon Hwang]

Created attachment 190515 [details]
WIP. discussion for APIs for CoordinatedGraphicsState

As we discussed in IRC, I made new patch for this issue.

The main focus of this patch is unifying coordinated graphic's messages into CoordinatedGraphicsState.

Each CoordinatedGraphicsLayer has a CoordiantedGraphicsLayerState, and whenever it needs sync, CoordinatedGraphicsLayer
commits to CoordinatedLayerTreeHost, which will store committed states from layers.

And CoordinatedLayerTreeHost sends stored states from layers, and state changes for global state using single command.
(current patch uses DidRenderFrame but it will be better to use CommitCooridnatedGraphicsState)
We can add preprocessing steps for IPC in LayerTreeCoordiantedHost.
The requirement from https://bugs.webkit.org/show_bug.cgi?id=109579 can be achieved using this.(#37)
(But I am not sure it is correct design decision)

To minimize IPC overhead, the size of encoded CoordinatedGraphicsLayerState is variable.
(CoordinatedGraphicsArgumentCoder checks dirty bit of CoordinatedGraphicsLayerState before encoding.) 

This patch is not working now, due to the bugs, but I think I need to share status.


I'll post working/clean version ASAP.

[Noam Rosenthal]

(In reply to comment #39)
> Created an attachment (id=190515) [details]
> WIP. discussion for APIs for CoordinatedGraphicsState
> 
> As we discussed in IRC, I made new patch for this issue.
> 
> The main focus of this patch is unifying coordinated graphic's messages into CoordinatedGraphicsState.
> 
> Each CoordinatedGraphicsLayer has a CoordiantedGraphicsLayerState, and whenever it needs sync, CoordinatedGraphicsLayer
> commits to CoordinatedLayerTreeHost, which will store committed states from layers.
> 
> And CoordinatedLayerTreeHost sends stored states from layers, and state changes for global state using single command.
> (current patch uses DidRenderFrame but it will be better to use CommitCooridnatedGraphicsState)
> We can add preprocessing steps for IPC in LayerTreeCoordiantedHost.
> The requirement from https://bugs.webkit.org/show_bug.cgi?id=109579 can be achieved using this.(#37)
> (But I am not sure it is correct design decision)
> 
> To minimize IPC overhead, the size of encoded CoordinatedGraphicsLayerState is variable.
> (CoordinatedGraphicsArgumentCoder checks dirty bit of CoordinatedGraphicsLayerState before encoding.) 
> 
> This patch is not working now, due to the bugs, but I think I need to share status.
> 
> 
> I'll post working/clean version ASAP.

Looks really good!

[Dongseong Hwang]

Comment on attachment 190515 [details]
WIP. discussion for APIs for CoordinatedGraphicsState

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

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:614
> +    m_layerState.contentsOpaque = contentsOpaque();

It is not needed because we already did in setContentsOpaque.
Below code ditto.

[Gwang Yoon Hwang]

Created attachment 191198 [details]
Patch

[Gwang Yoon Hwang]

(In reply to comment #41)
> (From update of attachment 190515 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=190515&action=review
> LGTM :)
> 
> > Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:614
> > +    m_layerState.contentsOpaque = contentsOpaque();
> 
> It is not needed because we already did in setContentsOpaque.
> Below code ditto.

Fixed.

[Gwang Yoon Hwang]

I post refined patch for review.

But I didn't remove UpdateAtlasClient and CoordinatedImageBacking::Coordinator
in this patch. I think that should be done in separated patch.

I'll make / post a bug & patch for that ASAP.

[Noam Rosenthal]

Comment on attachment 191198 [details]
Patch

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

I have some nitpicks, but otherwise I think this is ready for a WK2 owner to look at.

> Source/WebCore/ChangeLog:29
> +        No new tests, as it is a refactoring.

I would simply say that this is covered by existing tests.

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:451
> +    // TODO WHAT?

Probably something like m_layerState.imageBackingChanged = true.

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:613
> +    // State Properties

Unnecessary comment.

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsScene.h:79
> +    void flushStateChanges(const CoordinatedGraphicsState&);

Let's call this commitSceneState.

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsScene.h:105
> +    // Real Operations for flushStateChanges

Comment not needed.

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsState.h:142
> +    Vector<std::pair<uint32_t /* tileID */, float /* scale */> > tilesToCreate;

I don't like the use of std::pair here. Let's have a proper struct with tileID and scale.

> Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsArgumentCoders.cpp:880
> +    if (!decodeLayerStateIfNeeded(decoder, state.flags, state.flagsChanged))

I think the ifNeeded function is unnecessary. You can do
if (state.flagsChanged && !decoder.decode(state.flags))
and it would be as readable.

[Kenneth Rohde Christiansen]

Comment on attachment 191198 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        CoordinatedGraphicsScene (4 classes).
> +        If we want to add a new message, we need to add similar functions into 4 classes.
> +

Today, if we ... With this patch, that is not needed anymore

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsScene.cpp:354
> +        // Never make the root layer clip.

Never clip the root layer?

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsScene.cpp:617
> +    for (LayerUpdatePairVector::const_iterator iter = state.layersToUpdate.begin(); iter != end; ++iter)

not iter and not it which is more commonly used?

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsState.h:127
> +    {
> +    }
> +    FloatPoint pos;

newline after }

[Gwang Yoon Hwang]

Created attachment 191353 [details]
Patch

[Gwang Yoon Hwang]

(In reply to comment #45)
> (From update of attachment 191198 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=191198&action=review
> 
> I have some nitpicks, but otherwise I think this is ready for a WK2 owner to look at.
> 
> > Source/WebCore/ChangeLog:29
> > +        No new tests, as it is a refactoring.
> 
> I would simply say that this is covered by existing tests.
> 
That's good.

> > Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:451
> > +    // TODO WHAT?
> 
> Probably something like m_layerState.imageBackingChanged = true.
> 
I forgot to remove that line. :)
m_layerState.imageBackingChanged will be set in CoordinatedGraphicsLayer::syncImageBacking()

> > Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:613
> > +    // State Properties
> 
> Unnecessary comment.
>
Removed.
 
> > Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsScene.h:79
> > +    void flushStateChanges(const CoordinatedGraphicsState&);
> 
> Let's call this commitSceneState.
> 
Good naming.

> > Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsScene.h:105
> > +    // Real Operations for flushStateChanges
> 
> Comment not needed.
> 
Removed.

> > Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsState.h:142
> > +    Vector<std::pair<uint32_t /* tileID */, float /* scale */> > tilesToCreate;
> 
> I don't like the use of std::pair here. Let's have a proper struct with tileID and scale.
> 
I made struct TileCreationInfo. How about it?

> > Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsArgumentCoders.cpp:880
> > +    if (!decodeLayerStateIfNeeded(decoder, state.flags, state.flagsChanged))
> 
> I think the ifNeeded function is unnecessary. You can do
> if (state.flagsChanged && !decoder.decode(state.flags))
> and it would be as readable.
>
[de|en]codeLayerStateIfNeeded are removed, for the sake of consistency.



(In reply to comment #46)
> (From update of attachment 191198 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=191198&action=review
> 
> > Source/WebCore/ChangeLog:12
> > +        CoordinatedGraphicsScene (4 classes).
> > +        If we want to add a new message, we need to add similar functions into 4 classes.
> > +
> 
> Today, if we ... With this patch, that is not needed anymore
> 
AFAIK, that sentence describes a existing problem , which this patch should solve.
I think it is needed :)

> > Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsScene.cpp:354
> > +        // Never make the root layer clip.
> 
> Never clip the root layer?
Changed.


> > Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsScene.cpp:617
> > +    for (LayerUpdatePairVector::const_iterator iter = state.layersToUpdate.begin(); iter != end; ++iter)
> 
> not iter and not it which is more commonly used?
> 
I loosed my consistency. I prefer to use "it" for iterator. changed.

> > Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsState.h:127
> > +    {
> > +    }
> > +    FloatPoint pos;
> 
> newline after }
Fixed.

[Gwang Yoon Hwang]

Created attachment 191399 [details]
Patch

[Gwang Yoon Hwang]

UpdateImageBacking is not handled in posted patch. I will update patch soon.

[Noam Rosenthal]

Comment on attachment 191399 [details]
Patch

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

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsScene.cpp:490
> +    Vector<uint32_t>::const_iterator end = state.tilesToRemove.end();
> +    for (Vector<uint32_t>::const_iterator it = state.tilesToRemove.begin(); it != end; ++it)

We usually don't use Vector::iterator, instead just iterate using size() and an integer.

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsScene.cpp:502
> +    createTilesIfNeeded(layer, state);
> +    removeTilesIfNeeded(layer, state);
> +
> +    if (state.tilesToUpdate.isEmpty())
> +        return;

I would have createTilesIfNeeded, removeTilesIfNeeded and updateTilesIfNeeded called from setLayerState separately.

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsScene.cpp:508
> +    Vector<TileUpdateInfo>::const_iterator end = state.tilesToUpdate.end();
> +    for (Vector<TileUpdateInfo>::const_iterator it = state.tilesToUpdate.begin(); it != end; ++it) {

Ditto for Vector::iterator

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsScene.cpp:617
> +    typedef Vector<std::pair<CoordinatedLayerID, CoordinatedGraphicsLayerState> > LayerUpdatePairVector;
> +    LayerUpdatePairVector::const_iterator end = state.layersToUpdate.end();
> +    for (LayerUpdatePairVector::const_iterator it = state.layersToUpdate.begin(); it != end; ++it)

Ditto for Vector :)

> Source/WebKit2/Scripts/webkit2/messages.py:401
> +        'WebCore::CoordinatedGraphicsLayerState': ['<WebCore/CoordinatedGraphicsState.h>'],

This is not needed.

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.cpp:381
> +void CoordinatedLayerTreeHost::preprocessStateChange(CoordinatedGraphicsLayerState& state)
>  {
> -    m_shouldSyncFrame = true;
> -    m_webPage->send(Messages::CoordinatedLayerTreeHostProxy::SetCompositingLayerChildren(id, children));
> +    preprocessAnimationStateIfNeeded(state);
> +    preprocessFilterOperationsIfNeeded(state);
>  }

Really all you do here is update the custom filters.
I would call this function prepareCustomFilterProxiesIfNeeded, and guard it with CSS_SHADERS

[Gwang Yoon Hwang]

Created attachment 191429 [details]
Patch

[Gwang Yoon Hwang]

Created attachment 191430 [details]
Patch

[Gwang Yoon Hwang]

(In reply to comment #51)
> (From update of attachment 191399 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=191399&action=review
> 
> > Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsScene.cpp:490
> > +    Vector<uint32_t>::const_iterator end = state.tilesToRemove.end();
> > +    for (Vector<uint32_t>::const_iterator it = state.tilesToRemove.begin(); it != end; ++it)
> 
> We usually don't use Vector::iterator, instead just iterate using size() and an integer.
>
All of Vector::iterator has been removed in this patch.

> > Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsScene.cpp:502
> > +    createTilesIfNeeded(layer, state);
> > +    removeTilesIfNeeded(layer, state);
> > +
> > +    if (state.tilesToUpdate.isEmpty())
> > +        return;
> 
> I would have createTilesIfNeeded, removeTilesIfNeeded and updateTilesIfNeeded called from setLayerState separately.
>
Good point. Fixed.


> > Source/WebKit2/Scripts/webkit2/messages.py:401
> > +        'WebCore::CoordinatedGraphicsLayerState': ['<WebCore/CoordinatedGraphicsState.h>'],
> 
> This is not needed.
>
Removed.

> > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.cpp:381
> > +void CoordinatedLayerTreeHost::preprocessStateChange(CoordinatedGraphicsLayerState& state)
> >  {
> > -    m_shouldSyncFrame = true;
> > -    m_webPage->send(Messages::CoordinatedLayerTreeHostProxy::SetCompositingLayerChildren(id, children));
> > +    preprocessAnimationStateIfNeeded(state);
> > +    preprocessFilterOperationsIfNeeded(state);
> >  }
> 
> Really all you do here is update the custom filters.
> I would call this function prepareCustomFilterProxiesIfNeeded, and guard it with CSS_SHADERS

I made prepareCustomFilterProxiesIfNeeded, and removed preprocessAnimationsStateIfNeeded and preprocessFilterOperationsIfNeeded.

By the way, current code of CoordinatedLayerTreeHost::setLayerAnimations is weird.

717     GraphicsLayerAnimations activeAnimations = animations.getActiveAnimations();
718 #if ENABLE(CSS_SHADERS)
719     for (size_t i = 0; i < activeAnimations.animations().size(); ++i) {
720         const KeyframeValueList& keyframes = animations.animations().at(i).keyframes();
721         if (keyframes.property() != AnimatedPropertyWebkitFilter)
...
728 #endif
729     m_webPage->send(Messages::CoordinatedLayerTreeHostProxy::SetLayerAnimations(layerID, activeAnimations));
730 }

It uses size of activeAnimations.animations to iterate animations, but it iterates animations.animations() not activeAnimations.animations.

It seems we need only activeAnimations, so I modified to set activeAnimations on CoordinatedGraphicsLayerState.

[Noam Rosenthal]

(In reply to comment #54)
> (In reply to comment #51)
> > (From update of attachment 191399 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=191399&action=review
> > 
> > > Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsScene.cpp:490
> > > +    Vector<uint32_t>::const_iterator end = state.tilesToRemove.end();
> > > +    for (Vector<uint32_t>::const_iterator it = state.tilesToRemove.begin(); it != end; ++it)
> > 
> > We usually don't use Vector::iterator, instead just iterate using size() and an integer.
> >
> All of Vector::iterator has been removed in this patch.
> 
> > > Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsScene.cpp:502
> > > +    createTilesIfNeeded(layer, state);
> > > +    removeTilesIfNeeded(layer, state);
> > > +
> > > +    if (state.tilesToUpdate.isEmpty())
> > > +        return;
> > 
> > I would have createTilesIfNeeded, removeTilesIfNeeded and updateTilesIfNeeded called from setLayerState separately.
> >
> Good point. Fixed.
> 
> 
> > > Source/WebKit2/Scripts/webkit2/messages.py:401
> > > +        'WebCore::CoordinatedGraphicsLayerState': ['<WebCore/CoordinatedGraphicsState.h>'],
> > 
> > This is not needed.
> >
> Removed.
> 
> > > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.cpp:381
> > > +void CoordinatedLayerTreeHost::preprocessStateChange(CoordinatedGraphicsLayerState& state)
> > >  {
> > > -    m_shouldSyncFrame = true;
> > > -    m_webPage->send(Messages::CoordinatedLayerTreeHostProxy::SetCompositingLayerChildren(id, children));
> > > +    preprocessAnimationStateIfNeeded(state);
> > > +    preprocessFilterOperationsIfNeeded(state);
> > >  }
> > 
> > Really all you do here is update the custom filters.
> > I would call this function prepareCustomFilterProxiesIfNeeded, and guard it with CSS_SHADERS
> 
> I made prepareCustomFilterProxiesIfNeeded, and removed preprocessAnimationsStateIfNeeded and preprocessFilterOperationsIfNeeded.
> 
> By the way, current code of CoordinatedLayerTreeHost::setLayerAnimations is weird.
> 
> 717     GraphicsLayerAnimations activeAnimations = animations.getActiveAnimations();
> 718 #if ENABLE(CSS_SHADERS)
> 719     for (size_t i = 0; i < activeAnimations.animations().size(); ++i) {
> 720         const KeyframeValueList& keyframes = animations.animations().at(i).keyframes();
> 721         if (keyframes.property() != AnimatedPropertyWebkitFilter)
> ...
> 728 #endif
> 729     m_webPage->send(Messages::CoordinatedLayerTreeHostProxy::SetLayerAnimations(layerID, activeAnimations));
> 730 }
> 
> It uses size of activeAnimations.animations to iterate animations, but it iterates animations.animations() not activeAnimations.animations.
> 
> It seems we need only activeAnimations, so I modified to set activeAnimations on CoordinatedGraphicsLayerState.

OK, nice.
We need to have a WK2 owner look at the WK2 changes.

[Gwang Yoon Hwang]

(In reply to comment #55)
> OK, nice.
> We need to have a WK2 owner look at the WK2 changes.

Thanks for review.
I asked to andersca to look at this bug, but we will have a bunch of time lag. :)

[Anders Carlsson]

Comment on attachment 191430 [details]
Patch

WK2 parts look good to me.

[WebKit Review Bot]

Comment on attachment 191430 [details]
Patch

Clearing flags on attachment: 191430

Committed r144787: <http://trac.webkit.org/changeset/144787>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(In reply to comment #58)
> (From update of attachment 191430 [details])
> Clearing flags on attachment: 191430
> 
> Committed r144787: <http://trac.webkit.org/changeset/144787>

Buildfix landed for !USE(GRAPHICS_SURFACE) platforms - https://trac.webkit.org/changeset/144885

But unfortunately Qt-Mac build is still broken:
In file included from /Users/admin/work/WebKit-BuildSlave/qt-mountainlion-release/build/Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsArgumentCoders.cpp:29:
In file included from /Users/admin/work/WebKit-BuildSlave/qt-mountainlion-release/build/Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsArgumentCoders.h:32:
In file included from /Users/admin/work/WebKit-BuildSlave/qt-mountainlion-release/build/Source/WebKit2/Platform/CoreIPC/ArgumentCoders.h:29:
In file included from /Users/admin/work/WebKit-BuildSlave/qt-mountainlion-release/build/Source/WebKit2/Platform/CoreIPC/ArgumentDecoder.h:29:
/Users/admin/work/WebKit-BuildSlave/qt-mountainlion-release/build/Source/WebKit2/Platform/CoreIPC/ArgumentCoder.h:39:10: error: member reference base type 'const unsigned long' is not a structure or union
        t.encode(encoder);
        ~^~~~~~~
/Users/admin/work/WebKit-BuildSlave/qt-mountainlion-release/build/Source/WebKit2/Platform/CoreIPC/ArgumentEncoder.h:57:27: note: in instantiation of member function 'CoreIPC::ArgumentCoder<unsigned long>::encode' requested here
        ArgumentCoder<T>::encode(*this, t);
                          ^
/Users/admin/work/WebKit-BuildSlave/qt-mountainlion-release/build/Source/WebKit2/Platform/CoreIPC/ArgumentEncoder.h:62:9: note: in instantiation of function template specialization 'CoreIPC::ArgumentEncoder::encode<unsigned long>' requested here
        encode(t);
        ^
/Users/admin/work/WebKit-BuildSlave/qt-mountainlion-release/build/Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsArgumentCoders.cpp:1018:13: note: in instantiation of function template specialization 'CoreIPC::ArgumentEncoder::operator<<<unsigned long>' requested here
    encoder << state.imagesToUpdate.size();
            ^
In file included from /Users/admin/work/WebKit-BuildSlave/qt-mountainlion-release/build/Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsArgumentCoders.cpp:29:
In file included from /Users/admin/work/WebKit-BuildSlave/qt-mountainlion-release/build/Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsArgumentCoders.h:32:
In file included from /Users/admin/work/WebKit-BuildSlave/qt-mountainlion-release/build/Source/WebKit2/Platform/CoreIPC/ArgumentCoders.h:29:
In file included from /Users/admin/work/WebKit-BuildSlave/qt-mountainlion-release/build/Source/WebKit2/Platform/CoreIPC/ArgumentDecoder.h:29:
/Users/admin/work/WebKit-BuildSlave/qt-mountainlion-release/build/Source/WebKit2/Platform/CoreIPC/ArgumentCoder.h:44:16: error: type 'unsigned long' cannot be used prior to '::' because it has no members
        return T::decode(decoder, t);
               ^
/Users/admin/work/WebKit-BuildSlave/qt-mountainlion-release/build/Source/WebKit2/Platform/CoreIPC/ArgumentDecoder.h:90:34: note: in instantiation of member function 'CoreIPC::ArgumentCoder<unsigned long>::decode' requested here
        return ArgumentCoder<T>::decode(*this, t);
                                 ^
/Users/admin/work/WebKit-BuildSlave/qt-mountainlion-release/build/Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsArgumentCoders.cpp:1055:18: note: in instantiation of function template specialization 'CoreIPC::ArgumentDecoder::decode<unsigned long>' requested here
    if (!decoder.decode(sizeOfImagesToUpdate))
                 ^
2 errors generated.


cc-ing Qt maintainers, you can be interested in fixing this build failure.

[Dongseong Hwang]

This bug makes regression: A fixed element lags when scrolling and wheeling.
I filed and fixed in Bug 111829.