Summary: | [chromium] make WebTransformationMatrix object usable by non-webkit code | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Shawn Singh <shawnsingh> | ||||||||
Component: | Layout and Rendering | Assignee: | Shawn Singh <shawnsingh> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | abarth, cmarrin, dglazkov, eric, fishd, jamesr, tkent+wkapi, vollick, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 87788 | ||||||||||
Attachments: |
|
Description
Shawn Singh
2012-05-23 15:34:27 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. Created attachment 143686 [details]
Patch
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 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 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? Created attachment 144576 [details]
removed virtual, added comment, added compile assert
Thanks for the comments, here's the latest patch
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 Created attachment 144637 [details]
Patch for landing
Comment on attachment 144637 [details] Patch for landing Clearing flags on attachment: 144637 Committed r118857: <http://trac.webkit.org/changeset/118857> All reviewed patches have been landed. Closing bug. |