Bug 87510 – [chromium] create WebTransformOperation interface for chromium platform

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

Continue the work done in https://bugs.webkit.org/show_bug.cgi?id=86049 for transform operations.

Created attachment 144098 [details]
Patch

Created attachment 144123 [details]
Patch

Got rid of the type hierarchy. There is now only a WebTransformOperations object
that maintains a collection of WebTransformOperation structs (which are just a
matrix and a type).

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[WebKit Review Bot]

Attachment 144123 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/Platform/ChangeLog', u'Source/Platf..." exit_code: 1
Source/WebKit/chromium/tests/WebTransformOperationsTest.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[James Robinson]

Comment on attachment 144123 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=144123&action=review

Needs a touch more indirection but the API looks quite nice.

> Source/Platform/chromium/public/WebTransformOperations.h:30
> +#include <wtf/Vector.h>

this has to be hidden - users in the chromium repo don't have WTF/ on their include path so this won't work.

One option is to have a WebTransformOperationsPrivate that is forward declared here and held via a WebPrivateOwnPtr and have that class' impl hold a WTF::Vector

> Source/Platform/chromium/public/WebTransformOperations.h:51
> +    WebTransformationMatrix apply() const;

please add WEBKIT_EXPORT for functions declared here but defined in the .cpp

Created attachment 144355 [details]
Patch

(In reply to comment #5)
> (From update of attachment 144123 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=144123&action=review
>
> Needs a touch more indirection but the API looks quite nice.
>
> > Source/Platform/chromium/public/WebTransformOperations.h:30
> > +#include <wtf/Vector.h>
>
> this has to be hidden - users in the chromium repo don't have WTF/ on their include path so this won't work.
Done.
>
> One option is to have a WebTransformOperationsPrivate that is forward declared here and held via a WebPrivateOwnPtr and have that class' impl hold a WTF::Vector
>
> > Source/Platform/chromium/public/WebTransformOperations.h:51
> > +    WebTransformationMatrix apply() const;
>
> please add WEBKIT_EXPORT for functions declared here but defined in the .cpp
Done.

[WebKit Review Bot]

Attachment 144355 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/Platform/ChangeLog', u'Source/Platf..." exit_code: 1
Source/WebKit/chromium/tests/WebTransformOperationsTest.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[James Robinson]

Comment on attachment 144355 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=144355&action=review

> Source/Platform/chromium/public/WebTransformOperations.h:43
> +    WEBKIT_EXPORT WebTransformOperations();

it's a bit atypical to have exported c'tors / d'tors in the WebKit API - see "Methods" in http://trac.webkit.org/wiki/ChromiumWebKitAPI.  It's more typical to have inline c'tors that call exported functions if necessary

> Source/Platform/chromium/public/WebTransformOperations.h:46
> +    // Not virtual. Should not be subclassed.

This comment isn't really necessary, IMO

> Source/Platform/chromium/public/WebTransformOperations.h:75
> +    enum Type {

See "Enums" in http://trac.webkit.org/wiki/ChromiumWebKitAPI

Created attachment 144848 [details]
Patch

(In reply to comment #8)
> (From update of attachment 144355 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=144355&action=review
>
> > Source/Platform/chromium/public/WebTransformOperations.h:43
> > +    WEBKIT_EXPORT WebTransformOperations();
>
> it's a bit atypical to have exported c'tors / d'tors in the WebKit API - see "Methods" in http://trac.webkit.org/wiki/ChromiumWebKitAPI.  It's more typical to have inline c'tors that call exported functions if necessary
Done.
>
> > Source/Platform/chromium/public/WebTransformOperations.h:46
> > +    // Not virtual. Should not be subclassed.
>
> This comment isn't really necessary, IMO
Removed.
>
> > Source/Platform/chromium/public/WebTransformOperations.h:75
> > +    enum Type {
>
> See "Enums" in http://trac.webkit.org/wiki/ChromiumWebKitAPI
Fixed.

[WebKit Review Bot]

Attachment 144848 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/Platform/ChangeLog', u'Source/Platf..." exit_code: 1
Source/WebKit/chromium/tests/WebTransformOperationsTest.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[James Robinson]

Comment on attachment 144848 [details]
Patch

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?

Created attachment 144975 [details]
Patch

(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?

[WebKit Review Bot]

Attachment 144975 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/Platform/ChangeLog', u'Source/Platf..." exit_code: 1
Source/WebKit/chromium/tests/WebTransformOperationsTest.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Shawn Singh]

(In reply to comment #12)
> > > 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?


Just a side comment, as far as I know that _is_ the common header, but maybe we just should change the name at some point?   Maybe something like CCTestCommon.h.

[James Robinson]

Comment on attachment 144975 [details]
Patch

Both the name and the grab-bag nature are confusing to me.

R=me

[WebKit Review Bot]

Comment on attachment 144975 [details]
Patch

Clearing flags on attachment: 144975

Committed r119121: <http://trac.webkit.org/changeset/119121>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.