[chromium] Add conversion between WebTransformation and gfx::Transform
Created attachment 174791 [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 174791 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=174791&action=review Are you sure about adding these before we finish transitioning chromium code? I'm worried about accidentally adding copies. > Source/Platform/chromium/public/WebTransformationMatrix.h:160 > + gfx::Transform t; we don't use 1-letter variable names for anything but loop counters in WebKit
Comment on attachment 174791 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=174791&action=review > Source/Platform/chromium/public/WebTransformationMatrix.h:147 > + m_matrix[i][j] = t.matrix().getDouble(i, j); One of these should go "j, i" because you need to transpose to convert between WebKit matrices and Skia matrices.
Created attachment 174793 [details] Patch
Comment on attachment 174791 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=174791&action=review >> Source/Platform/chromium/public/WebTransformationMatrix.h:147 >> + m_matrix[i][j] = t.matrix().getDouble(i, j); > > One of these should go "j, i" because you need to transpose to convert between WebKit matrices and Skia matrices. Oh snap, I thought I checked and it wasn't. Here's translateX WebKit: m_matrix[3][0] Skia (via gfx): matrix_.set(0, 3, SkFloatToScalar(x)) You're right. I misread! Will fix. >> Source/Platform/chromium/public/WebTransformationMatrix.h:160 >> + gfx::Transform t; > > we don't use 1-letter variable names for anything but loop counters in WebKit Oops, fixing.
Created attachment 174794 [details] Patch
(In reply to comment #3) > (From update of attachment 174791 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=174791&action=review > > Are you sure about adding these before we finish transitioning chromium code? I'm worried about accidentally adding copies. I am expecting the cc/ conversion to be ~1 patch, but it could be more than one, and then we should be careful. So, we can explicitly convert somewhere else, then delete it and add implicit conversions here. Or, we can have explicit ones here, and then make them implicit later. I leave that to your discretion. I like them here because it's where the code is going to end up eventually anyways.
Comment on attachment 174794 [details] Patch I'd be fine with having these implicit if we can complete the conversion fast enough (1 patch would be ideal). I don't want to get caught out in an intermediate state for too long. R=me, up to you on the implicit/explicit
Comment on attachment 174794 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=174794&action=review > Source/Platform/chromium/public/WebTransformationMatrix.h:148 > + m_matrix[i][j] = transform.matrix().getDouble(j, i); Maybe use SkMatrix44::asColMajord or asRowMajord?
(In reply to comment #9) > (From update of attachment 174794 [details]) > I'd be fine with having these implicit if we can complete the conversion fast enough (1 patch would be ideal). I don't want to get caught out in an intermediate state for too long. > > R=me, up to you on the implicit/explicit I do like being safe! And don't mind following up on this after. Thanks for the review.
Comment on attachment 174794 [details] Patch Attachment 174794 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14857848 New failing tests: inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html
Comment on attachment 174794 [details] Patch Pretty sure this isn't crashing a test. Let's try this again.
Comment on attachment 174794 [details] Patch Clearing flags on attachment: 174794 Committed r135031: <http://trac.webkit.org/changeset/135031>
All reviewed patches have been landed. Closing bug.