[webkit-reviews] review denied: [Bug 80054] [chromium] Expose compositor filters to Aura through WebLayer : [Attachment 137439] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Apr 16 17:42:20 PDT 2012


James Robinson <jamesr at chromium.org> has denied Dana Jansens
<danakj at chromium.org>'s request for review:
Bug 80054: [chromium] Expose compositor filters to Aura through WebLayer
https://bugs.webkit.org/show_bug.cgi?id=80054

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

------- Additional Comments from James Robinson <jamesr at chromium.org>
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


More information about the webkit-reviews mailing list