[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