Bug 87510 - [chromium] create WebTransformOperation interface for chromium platform
Summary: [chromium] create WebTransformOperation interface for chromium platform
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: vollick
URL:
Keywords:
Depends on:
Blocks: 87686 87788
  Show dependency treegraph
 
Reported: 2012-05-25 10:17 PDT by vollick
Modified: 2012-05-31 10:38 PDT (History)
7 users (show)

See Also:


Attachments
Patch (66.15 KB, patch)
2012-05-25 10:34 PDT, vollick
no flags Details | Formatted Diff | Diff
Patch (22.08 KB, patch)
2012-05-25 12:29 PDT, vollick
no flags Details | Formatted Diff | Diff
Patch (24.33 KB, patch)
2012-05-28 07:19 PDT, vollick
no flags Details | Formatted Diff | Diff
Patch (27.00 KB, patch)
2012-05-30 10:02 PDT, vollick
no flags Details | Formatted Diff | Diff
Patch (26.50 KB, patch)
2012-05-30 19:57 PDT, vollick
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description vollick 2012-05-25 10:17:50 PDT
Continue the work done in https://bugs.webkit.org/show_bug.cgi?id=86049 for transform operations.
Comment 1 vollick 2012-05-25 10:34:33 PDT
Created attachment 144098 [details]
Patch
Comment 2 vollick 2012-05-25 12:29:31 PDT
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).
Comment 3 WebKit Review Bot 2012-05-25 12:31:29 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 4 WebKit Review Bot 2012-05-25 12:31:47 PDT
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.
Comment 5 James Robinson 2012-05-25 13:20:16 PDT
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
Comment 6 vollick 2012-05-28 07:19:01 PDT
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.
Comment 7 WebKit Review Bot 2012-05-28 07:21:55 PDT
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.
Comment 8 James Robinson 2012-05-29 17:45:57 PDT
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
Comment 9 vollick 2012-05-30 10:02:22 PDT
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.
Comment 10 WebKit Review Bot 2012-05-30 10:08:12 PDT
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.
Comment 11 James Robinson 2012-05-30 19:03:47 PDT
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?
Comment 12 vollick 2012-05-30 19:57:56 PDT
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?
Comment 13 WebKit Review Bot 2012-05-30 20:03:11 PDT
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.
Comment 14 Shawn Singh 2012-05-31 10:13:12 PDT
(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.
Comment 15 James Robinson 2012-05-31 10:16:10 PDT
Comment on attachment 144975 [details]
Patch

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

R=me
Comment 16 WebKit Review Bot 2012-05-31 10:38:24 PDT
Comment on attachment 144975 [details]
Patch

Clearing flags on attachment: 144975

Committed r119121: <http://trac.webkit.org/changeset/119121>
Comment 17 WebKit Review Bot 2012-05-31 10:38:30 PDT
All reviewed patches have been landed.  Closing bug.