Bug 87315 - [chromium] make WebTransformationMatrix object usable by non-webkit code
Summary: [chromium] make WebTransformationMatrix object usable by non-webkit code
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Shawn Singh
URL:
Keywords:
Depends on:
Blocks: 87788
  Show dependency treegraph
 
Reported: 2012-05-23 15:34 PDT by Shawn Singh
Modified: 2012-05-29 16:42 PDT (History)
9 users (show)

See Also:


Attachments
Patch (18.32 KB, patch)
2012-05-23 17:04 PDT, Shawn Singh
no flags Details | Formatted Diff | Diff
removed virtual, added comment, added compile assert (19.11 KB, patch)
2012-05-29 10:05 PDT, Shawn Singh
no flags Details | Formatted Diff | Diff
Patch for landing (19.31 KB, patch)
2012-05-29 16:07 PDT, Shawn Singh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Shawn Singh 2012-05-23 15:34:27 PDT
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.
Comment 1 Shawn Singh 2012-05-23 17:03:40 PDT
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.
Comment 2 Shawn Singh 2012-05-23 17:04:21 PDT
Created attachment 143686 [details]
Patch
Comment 3 WebKit Review Bot 2012-05-23 17:08:13 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 James Robinson 2012-05-23 18:16:28 PDT
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
Comment 5 Darin Fisher (:fishd, Google) 2012-05-23 20:15:25 PDT
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?
Comment 6 Shawn Singh 2012-05-29 10:05:34 PDT
Created attachment 144576 [details]
removed virtual, added comment, added compile assert

Thanks for the comments, here's the latest patch
Comment 7 James Robinson 2012-05-29 15:50:36 PDT
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
Comment 8 Shawn Singh 2012-05-29 16:07:56 PDT
Created attachment 144637 [details]
Patch for landing
Comment 9 WebKit Review Bot 2012-05-29 16:42:44 PDT
Comment on attachment 144637 [details]
Patch for landing

Clearing flags on attachment: 144637

Committed r118857: <http://trac.webkit.org/changeset/118857>
Comment 10 WebKit Review Bot 2012-05-29 16:42:50 PDT
All reviewed patches have been landed.  Closing bug.