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

[bugzilla-daemon at webkit.org]

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 137097: Patch
https://bugs.webkit.org/attachment.cgi?id=137097&action=review

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