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

[bugzilla-daemon at webkit.org]

vollick at chromium.org has asked	for review:
Bug 87510: [chromium] create WebTransformOperation interface for chromium
platform
https://bugs.webkit.org/show_bug.cgi?id=87510

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

------- Additional Comments from vollick at chromium.org
(In reply to comment #11)
> (From update of attachment 144848 [details])
> 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?
I think I need to include the header since I'm returning
WebTransformationMatrix by value.

>
> > 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?
It was so that I could pass NULL from the default constructor. I've created a
second initialize function for that case, and changed this one to accept a
reference.
>
> can reset()/initialize() be private? who needs to call them?
Fixed. The reason I'd done this was that the spec mentioned that the helper
functions called by the inline ctor/dtor should be decorated with WEBKIT_EXPORT
and this felt strange for private methods.
> > 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
Done.
>
> it's also more typical for opaque forward-declared things to be class instead
of struct
Done.
>
> > Source/Platform/chromium/public/WebTransformOperations.h:85
> > +
>
> nit: extra newline here
Removed.
>
> > 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
Hehe, it's my editor. I've gotten rid of this noise.
>
> > 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
It used to be in Platform/src and it was working nicely, but it stopped
compiling after I pulled/rebased. The problem is that
WebTransformOperations.cpp file includes WebTransformationMatrix.h which
includes TransformationMatrix.h, and I can't seem to access that header from
Platform/src, so I moved the .cpp into support with
WebTransformationMatrix.cpp.
>
> >> 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?
Yes, exactly. I'm getting the macro from there. Should I move the macro to a
more common header?