Bug 80054 – [chromium] Expose compositor filters to Aura through WebLayer

• Component: New Bugs
• Status: RESOLVED FIXED

[Dana Jansens]

[chromium] Expose compositor filters to Aura through WebLayer

[Dana Jansens]

Created attachment 129754 [details]
Patch

[Dana Jansens]

note to self: add comment on setBackgroundFilters() saying you can only do this for layers that render into the root layer.

[Dana Jansens]

Created attachment 137097 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[James Robinson]

Comment on attachment 137097 [details]
Patch

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

> Source/Platform/chromium/public/WebFilterOperations.h:38
> +// Simple wrapper around WebCore::FilterOperations. There are more (complex) operations possible
> +// in WebCore::FilterOperation subclasses, and this API should evolve when they are desired.

People using the WebKit API shouldn't need to know or care about WebCore, so API comments should describe what this class is and not refer to WebCore concepts.

> Source/Platform/chromium/public/WebFilterOperations.h:54
> +    void setSepia(double amount) { m_sepia = amount; }

Should this class just be a struct? Do we expect it to grow any interesting functionality, or just be a data bag?

> Source/Platform/chromium/public/WebLayer.h:99
> +    WEBKIT_EXPORT void setFilters(const WebFilterOperations&);

How do you clear the filters? setFilters() with a default-initialized WebFilterOperations?

> Source/Platform/chromium/public/WebLayer.h:102
> +    // filtered layer are capable of doing. CCLayerTreeHostCommon.cpp determines

The user of the public API shouldn't need to know or care about implementation details like CCLayerTreeHostCommon. Here you should describe what the restrictions are in terms of public API concepts.

What happens if someone calls this API on a layer that doesn't meet this criteria? Do we just silently drop their filters on the floor?

> Source/WebKit/chromium/public/platform/WebFilterOperations.h:26
> +#include "../../../../Platform/chromium/public/WebFilterOperations.h"

You don't need this. We only add forwarding headers into WebKit/chromium/public/** when:
1.) There is already existing code in chromium that #includes the header via this path
2.) The API is logically used as part of the client API and the platform API

Neither apply here

> Source/WebKit/chromium/src/WebFilterOperations.cpp:27
> +#include "platform/WebFilterOperations.h"

This file really belongs in Source/WebCore/platform/chromium/support/ - that's where implementations of the Platform API that depend only on WebCore concepts (and not existing Source/WebKit/chromium/src concepts) should go

#include platform API headers with the syntax <public/WebFilterOperations.h>

> Source/WebKit/chromium/src/WebFilterOperations.cpp:38
> +    if (m_sepia)
> +        filters.operations().append(BasicColorMatrixFilterOperation::create(m_sepia, FilterOperation::SEPIA));

Does the order in which the filters apply here matter? Can you document this behavior in the WebFilterOperations header?

> Source/WebKit/chromium/src/WebLayer.cpp:29
> +#include "platform/WebFilterOperations.h"

include Platform API headers as <public/WebFilterOperations.h> (the other #includes in here aren't all normative, but I plan to fix them at some point)

[Dana Jansens]

Created attachment 137172 [details]
Patch

[Dana Jansens]

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

>> Source/Platform/chromium/public/WebFilterOperations.h:38
>> +// in WebCore::FilterOperation subclasses, and this API should evolve when they are desired.
> 
> People using the WebKit API shouldn't need to know or care about WebCore, so API comments should describe what this class is and not refer to WebCore concepts.

done.

>> Source/Platform/chromium/public/WebFilterOperations.h:54
>> +    void setSepia(double amount) { m_sepia = amount; }
> 
> Should this class just be a struct? Do we expect it to grow any interesting functionality, or just be a data bag?

It puts the filters in an arbitrary order right now. If you are just doing a blur that's fine, but I'm imagining in the future the UI people might want something a bit better here.

>> Source/Platform/chromium/public/WebLayer.h:99
>> +    WEBKIT_EXPORT void setFilters(const WebFilterOperations&);
> 
> How do you clear the filters? setFilters() with a default-initialized WebFilterOperations?

Yes. Comment added.

>> Source/Platform/chromium/public/WebLayer.h:102
>> +    // filtered layer are capable of doing. CCLayerTreeHostCommon.cpp determines
> 
> The user of the public API shouldn't need to know or care about implementation details like CCLayerTreeHostCommon. Here you should describe what the restrictions are in terms of public API concepts.
> 
> What happens if someone calls this API on a layer that doesn't meet this criteria? Do we just silently drop their filters on the floor?

Yes, we just don't apply the filters. I didn't want to duplicate the code logic of CCLTHC here as the criteria could change over time. Modified comment to explain this in a more general way and say that we drop the filters.

>> Source/WebKit/chromium/public/platform/WebFilterOperations.h:26
>> +#include "../../../../Platform/chromium/public/WebFilterOperations.h"
> 
> You don't need this. We only add forwarding headers into WebKit/chromium/public/** when:
> 1.) There is already existing code in chromium that #includes the header via this path
> 2.) The API is logically used as part of the client API and the platform API
> 
> Neither apply here

Thanks, done.

>> Source/WebKit/chromium/src/WebFilterOperations.cpp:27
>> +#include "platform/WebFilterOperations.h"
> 
> This file really belongs in Source/WebCore/platform/chromium/support/ - that's where implementations of the Platform API that depend only on WebCore concepts (and not existing Source/WebKit/chromium/src concepts) should go
> 
> #include platform API headers with the syntax <public/WebFilterOperations.h>

done.

>> Source/WebKit/chromium/src/WebFilterOperations.cpp:38
>> +        filters.operations().append(BasicColorMatrixFilterOperation::create(m_sepia, FilterOperation::SEPIA));
> 
> Does the order in which the filters apply here matter? Can you document this behavior in the WebFilterOperations header?

Yeh it's arbitrary and I expect this to possibly matter in the distant future, but not now.

Done.

>> Source/WebKit/chromium/src/WebLayer.cpp:29
>> +#include "platform/WebFilterOperations.h"
> 
> include Platform API headers as <public/WebFilterOperations.h> (the other #includes in here aren't all normative, but I plan to fix them at some point)

k

[James Robinson]

Comment on attachment 137172 [details]
Patch

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

> Source/Platform/chromium/public/WebFilterOperations.h:38
> +// A simple set of filter operations. This implementation does not currently
> +// support defining an order for the operations.

This seems unnecessarily restrictive.  I think having WebFilterOperations be a list of WebFilterOperation instances would make a lot more sense and be more powerful.  Then a caller would only have to construct the WebFilterOperation instances that make sense for their use and add them to a WebFilterOperations set to pass to the layer.  You would only have to define the WebFilterOperation types that you need and WebFilterOperations could still convert easily to a WebCore::FilterOperations.

> Source/Platform/chromium/public/WebLayer.h:102
> +    // Background filters are only possible on layers that are drawn directly

nit: newline between function and comments for the next function

[Dana Jansens]

Comment on attachment 137172 [details]
Patch

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

>> Source/Platform/chromium/public/WebFilterOperations.h:38
>> +// support defining an order for the operations.
> 
> This seems unnecessarily restrictive.  I think having WebFilterOperations be a list of WebFilterOperation instances would make a lot more sense and be more powerful.  Then a caller would only have to construct the WebFilterOperation instances that make sense for their use and add them to a WebFilterOperations set to pass to the layer.  You would only have to define the WebFilterOperation types that you need and WebFilterOperations could still convert easily to a WebCore::FilterOperations.

I agree. I did this because WebVector is essentially read-only. You create it from a WTF::Vector, and then you can only read the WebVector. Do you want me to implement a better WebVector or do you have some other idea of what I could do here?

>> Source/Platform/chromium/public/WebLayer.h:102
>> +    // Background filters are only possible on layers that are drawn directly
> 
> nit: newline between function and comments for the next function

k

[James Robinson]

You could add API to WebFilterOperations that allowed you to append a WebFilterOperation and then let the implementation figure out how to assemble that into a data structure for marshalling to a WebCore::FilterOperations.  You don't have to provide the user of the API direct access to the WebFilterOperations' internal buffer.

[Dana Jansens]

Can I stick an internal buffer on WebFilterOperations that is not a WebVector? Maybe by storing a WTF::Vector* in a void* member variable of the class?

If I have to append to WebVector it means implementing append to an array.

[James Robinson]

You can have a backing that's opaque in the API that is whatever you like.  You could even have the backing be a WebCore::FilterOperations if you want.  Many WebKit APIs use the pattern of having an m_private pointer to internal data that is not exposed to the users of the API directly but is used to keep storage for the implementation.

[Dana Jansens]

Created attachment 137439 [details]
Patch

[Dana Jansens]

(In reply to comment #12)
> You can have a backing that's opaque in the API that is whatever you like.  You could even have the backing be a WebCore::FilterOperations if you want.  Many WebKit APIs use the pattern of having an m_private pointer to internal data that is not exposed to the users of the API directly but is used to keep storage for the implementation.

Thanks for this. We have something now that wraps WebCore::FilterOperations without introducing miles of boilerplate.

[James Robinson]

Comment on attachment 137439 [details]
Patch

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

This looks much better.  I think we need another round, though - getting the dependencies, etc, set up just right for publicly exposed API is tricky and requires some extra care.

> Source/Platform/chromium/public/WebFilterOperation.h:33
> +#include "FilterOperation.h"

this #include is unfortunate - it means that anything that sets WEBKIT_IMPLEMENTATION and #includes this file has to have WebCore headers on the include path, even if it shouldn't logically see most of WebCore (for instance if it's a Platform implementation in Source/Platform/chromium/src).  I can't really tell why it's here - it seems like everything in this header should compile fine with just a forward declaration of FilterOperation, right?

> Source/Platform/chromium/public/WebFilterOperation.h:50
> +class WebBasicColorMatrixFilterOperation : public WebFilterOperation {

Yes, this is exactly what I had in mind.  However, I think that these operations should all be structs with exposed members.  For users of the public API (i.e. people who don't know or care about stuff inside #if WEBKIT_IMPLEMENTATION) each of these operation instances is just a dumb data bag, and I imagine they'll stay that way.

> Source/Platform/chromium/public/WebFilterOperation.h:52
> +    enum Type {

Please see the Chromium WebKit API guide for enum naming:
http://trac.webkit.org/wiki/ChromiumWebKitAPI#Enums

I know it's not really consistent with how we do enums anywhere else, but we should stick to it.  We should add the checks to AssertMatchingEnums.cpp as the wiki page suggests - doing that will let you simplify the implementation a bit

> Source/Platform/chromium/public/WebFilterOperations.h:33
> +#include "FilterOperations.h"

We try to avoid #include'ing files in WebCore directly from Platform/chromium/public headers wherever possible, it creates some awkward include path situations.

You could avoid this by exposing the WebCore::FilterOperations getter by reference or pointer instead of by value so that the existing forward declaration is sufficient.

> Source/WebCore/platform/chromium/support/WebFilterOperation.cpp:38
> +    case Grayscale:
> +        return BasicColorMatrixFilterOperation::create(m_amount, FilterOperation::GRAYSCALE);

if you make the enum values match (see earlier comment) then you won't need these somewhat verbose switch blocks

[Dana Jansens]

Comment on attachment 137439 [details]
Patch

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

That's fine, thanks for the fast review :)

>> Source/Platform/chromium/public/WebFilterOperation.h:33
>> +#include "FilterOperation.h"
> 
> this #include is unfortunate - it means that anything that sets WEBKIT_IMPLEMENTATION and #includes this file has to have WebCore headers on the include path, even if it shouldn't logically see most of WebCore (for instance if it's a Platform implementation in Source/Platform/chromium/src).  I can't really tell why it's here - it seems like everything in this header should compile fine with just a forward declaration of FilterOperation, right?

Oh, yes.

>> Source/Platform/chromium/public/WebFilterOperation.h:52
>> +    enum Type {
> 
> Please see the Chromium WebKit API guide for enum naming:
> http://trac.webkit.org/wiki/ChromiumWebKitAPI#Enums
> 
> I know it's not really consistent with how we do enums anywhere else, but we should stick to it.  We should add the checks to AssertMatchingEnums.cpp as the wiki page suggests - doing that will let you simplify the implementation a bit

Should the enum still live inside the class? ie. WebBasicColorMatrixFilterOperation::WebBasicColorMatrixFilterOperationGrayscale?

>> Source/Platform/chromium/public/WebFilterOperations.h:33
>> +#include "FilterOperations.h"
> 
> We try to avoid #include'ing files in WebCore directly from Platform/chromium/public headers wherever possible, it creates some awkward include path situations.
> 
> You could avoid this by exposing the WebCore::FilterOperations getter by reference or pointer instead of by value so that the existing forward declaration is sufficient.

k

[Dana Jansens]

Comment on attachment 137439 [details]
Patch

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

>> Source/Platform/chromium/public/WebFilterOperation.h:50
>> +class WebBasicColorMatrixFilterOperation : public WebFilterOperation {
> 
> Yes, this is exactly what I had in mind.  However, I think that these operations should all be structs with exposed members.  For users of the public API (i.e. people who don't know or care about stuff inside #if WEBKIT_IMPLEMENTATION) each of these operation instances is just a dumb data bag, and I imagine they'll stay that way.

done.

>>> Source/Platform/chromium/public/WebFilterOperation.h:52
>>> +    enum Type {
>> 
>> Please see the Chromium WebKit API guide for enum naming:
>> http://trac.webkit.org/wiki/ChromiumWebKitAPI#Enums
>> 
>> I know it's not really consistent with how we do enums anywhere else, but we should stick to it.  We should add the checks to AssertMatchingEnums.cpp as the wiki page suggests - doing that will let you simplify the implementation a bit
> 
> Should the enum still live inside the class? ie. WebBasicColorMatrixFilterOperation::WebBasicColorMatrixFilterOperationGrayscale?

Ok so I see lots of enums in AssertMatchingEnums.cpp that are "enum WebBlahBlahFoo" because they are not in a class. But I also see a lot (more than half?) that are inside a class like WebBlahBlah::Foo.

So I gather that the latter is ok, as long as everything in the Foo enum is named FooStuff, FooStuff2..

[Dana Jansens]

Created attachment 137454 [details]
Patch

[James Robinson]

Comment on attachment 137454 [details]
Patch

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

R=me with a few minor things to address before landing

> Source/Platform/chromium/public/WebFilterOperation.h:30
> +#include "WebColor.h"
> +#include "WebCommon.h"

WebCommon.h should always come first (sort of like config.h in .cpp files)

> Source/Platform/chromium/public/WebFilterOperation.h:96
> +    WebBlurFilterOperation(int pixelRadius) : pixelRadius(pixelRadius) { }

explicit on 1-arg c'tors, please

> Source/Platform/chromium/public/WebLayer.h:106
> +    // Clear the filters in use by passing in a newly instantiated
> +    // WebFilterOperations object.
> +    WEBKIT_EXPORT void setFilters(const WebFilterOperations&);
> +
> +    // Background filters are only possible on layers that are drawn directly
> +    // to the root render surface. This means if an ancestor of the background-
> +    // filtered layer sets certain properties (opacity, transforms), it may
> +    // conflict and hide the background filters.

Instead of just describing the exceptional cases, can you please describe what these APIs do?  setFilters is fairly obvious, but for background filter it's not really clear at all what the actual result of this is.  I think a short description would go a long way

[Dana Jansens]

Comment on attachment 137454 [details]
Patch

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

ty

>> Source/Platform/chromium/public/WebFilterOperation.h:30
>> +#include "WebCommon.h"
> 
> WebCommon.h should always come first (sort of like config.h in .cpp files)

k

>> Source/Platform/chromium/public/WebFilterOperation.h:96
>> +    WebBlurFilterOperation(int pixelRadius) : pixelRadius(pixelRadius) { }
> 
> explicit on 1-arg c'tors, please

oops

>> Source/Platform/chromium/public/WebLayer.h:106
>> +    // conflict and hide the background filters.
> 
> Instead of just describing the exceptional cases, can you please describe what these APIs do?  setFilters is fairly obvious, but for background filter it's not really clear at all what the actual result of this is.  I think a short description would go a long way

k

[Dana Jansens]

Created attachment 137462 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 137462 [details]
Patch for landing

Clearing flags on attachment: 137462

Committed r114343: <http://trac.webkit.org/changeset/114343>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.