[webkit-reviews] review denied: [Bug 87510] [chromium] create WebTransformOperation interface for chromium platform : [Attachment 144848] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied vollick at chromium.org's request
for review:
Bug 87510: [chromium] create WebTransformOperation interface for chromium
platform
https://bugs.webkit.org/show_bug.cgi?id=87510

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

------- Additional Comments from James Robinson <jamesr at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=144848&action=review


Looks really good, just enough nits to need one more round but it's really
close.

> Source/Platform/chromium/public/WebTransformOperations.h:28
> +#include <public/WebTransformationMatrix.h>

I think you just need a forward declaration of WebTransformationMatrix - no?

> Source/Platform/chromium/public/WebTransformOperations.h:54
> +    WEBKIT_EXPORT void reset();
> +    WEBKIT_EXPORT void initialize(const WebTransformOperations* prototype);

why does this take a const pointer instead of a const reference?

can reset()/initialize() be private? who needs to call them?

> Source/Platform/chromium/public/WebTransformOperations.h:80
> +    struct WebTransformOperationsPrivate;

We would typically forward declare this outside the class so you don't have to
end up writing WebKit::WebTransformOperations::WebTransformOperationsPrivate

it's also more typical for opaque forward-declared things to be class instead
of struct

> Source/Platform/chromium/public/WebTransformOperations.h:85
> +

nit: extra newline here

> Source/WebCore/WebCore.gypi:6642
> +	       'inspector/front-end/Images/searchPrev.png',

you probably want to teach your editor to not edit unrelated lines like this
(or just suppress OCD urges to kill trailing whitespace), changes like this
cause unnecessary churn and risk of merge conflicts

> Source/WebCore/platform/chromium/support/WebTransformOperations.cpp:1
> +/*

Why is this implemented in WebCore/platform/.... ? we only do that when the
implementation depends on some WebCore/platform concepts, but it looks like
this doesn't and the .cpp should go in Platform/chromium/src

>> Source/WebKit/chromium/tests/WebTransformOperationsTest.cpp:30
>> +
> 
> Alphabetical sorting problem.  [build/include_order] [4]

I'll fix this style check someday....

> Source/WebKit/chromium/tests/WebTransformOperationsTest.cpp:31
> +#include "CCLayerTreeTestCommon.h"

This is an odd dependency - what comes out of this header? the transform_matrix
macro?