Bug 87315 – [chromium] make WebTransformationMatrix object usable by non-webkit code

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Shawn Singh]

There are two ways to do this.  Given the pros and cons, I'm opting for #1 since its a smaller change, and the end-state will be the same in both cases so I don't think it really matters.

Vollick@ FYI you'll encounter the same question when wrapping TransformOperations, KeframeList and Animation objects, and I think its trivial enough to do it in the first patch rather than how I did it in two separate patches =)


(1) use "placement new" syntax on the m_private pointer.

pros:
  much simpler code change
cons:
  requires a pointer and double[4][4], therefore will have awkward 68 or 72 byte size, as opposed to nice 64 byte size.
  the size of this data structure will have to change before migration is complete


(2) use #if WEBKIT_IMPLEMENTATION to define a transformationMatrix or a double[4][4], depending on which side of the code we're on.

pros:
  data structure remains the same size
  removes any pointer craziness in the class
cons:
  requires including TransformationMatrix.h in the WebTransformationMatrix.h file.  (guarded by #if WEBKIT_IMPLEMENTATION)
  larger code change from what it is now.

[Shawn Singh]

Please disregard my above pros and cons, the are not fully correct.

It turns out I mis-understood what was really involved if we used the placement new approach, there was a big con that we would have to initialize a new local copy of WebCore::TransformationMatrix for every function, do the computation, and copy the data back to the WebTransformationMatrix.

Instead, it works nicely to do option #2, though the pros and cons still dno't really apply.

For sanity, I also did a quick hacky test, in chromium code that did not have WEBKIT_IMPLEMENTATION defined (render_widget.cc), and made sure WebTransformationMatrix object could be instantiated and used correctly.  It worked just fine.

[Shawn Singh]

Created attachment 143686 [details]
Patch

[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.

[James Robinson]

Comment on attachment 143686 [details]
Patch

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

> Source/Platform/chromium/public/WebTransformationMatrix.h:51
> +    virtual ~WebTransformationMatrix() { }

I think no need for the virtual here

[Darin Fisher (:fishd, Google)]

Comment on attachment 143686 [details]
Patch

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

> Source/Platform/chromium/public/WebTransformationMatrix.h:152
> +#if WEBKIT_IMPLEMENTATION

this probably deserves a comment.  shouldn't you also add a COMPILE_ASSERT ensuring that the sizes are the same?

[Shawn Singh]

Created attachment 144576 [details]
removed virtual, added comment, added compile assert

Thanks for the comments, here's the latest patch

[James Robinson]

Comment on attachment 144576 [details]
removed virtual, added comment, added compile assert

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

> Source/Platform/chromium/public/WebTransformationMatrix.h:52
>      void reset();

do we still need a reset()? looks like there's no implementation or caller of it now and we have makeIdentity() to turn an instance into a default-constructed instance

> Source/WebCore/platform/chromium/support/WebTransformationMatrix.cpp:67
> +    bool isEqual = (m_private == t.m_private);
>      return isEqual;

I would just do:

return m_private == other.m_private

[Shawn Singh]

Created attachment 144637 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 144637 [details]
Patch for landing

Clearing flags on attachment: 144637

Committed r118857: <http://trac.webkit.org/changeset/118857>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.