Summary: | Simplify TransformationMatrix rotation code to improve precision | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Shawn Singh <shawnsingh> | ||||
Component: | Layout and Rendering | Assignee: | Shawn Singh <shawnsingh> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | cmarrin, dino, enne, webkit.review.bot | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Shawn Singh
2012-05-16 13:09:46 PDT
Created attachment 144691 [details]
Patch
Tested with webkit_unit_tests and layout tests on mac; no obvious regressions. But I would prefer to run layout tests a few more times with/without this patch to double check that some of the flakiness was not the fault of this patch.
+enne for the webkit_unit_tests parts of this patch. > - There is a chance this will require rebaselining a vast number of pixel tests, and I would need some suggestions how to minimize the risk and pain of breaking all pixel tests for a short while.
last comment for now... I was completely wrong about this, nothing should need rebaselining. =)
Comment on attachment 144691 [details]
Patch
Why a Chromium-specific test for a cross-platform bug fix?
(In reply to comment #4) > (From update of attachment 144691 [details]) > Why a Chromium-specific test for a cross-platform bug fix? Looking at Shawn's test, it would be extremely awkward to try to expose that code into a cross-platform layout test. In my opinion, unit tests seem like the best way to test a change like this. If other ports had C++ unit tests, it certainly could be cross-platform. (In reply to comment #5) > (In reply to comment #4) > > (From update of attachment 144691 [details] [details]) > > Why a Chromium-specific test for a cross-platform bug fix? > > Looking at Shawn's test, it would be extremely awkward to try to expose that code into a cross-platform layout test. In my opinion, unit tests seem like the best way to test a change like this. > > If other ports had C++ unit tests, it certainly could be cross-platform. It's also worth pointing out that this change is already covered by existing layout tests, specifically transforms/cssmatrix-3d-interface.xhtml which did a great job catching a subtle mistake I had before submitting this patch. Comment on attachment 144691 [details]
Patch
Increasing precision seems non-controversial and your math looks correct. R=me.
Comment on attachment 144691 [details]
Patch
Thanks for reviewing!
Comment on attachment 144691 [details] Patch Clearing flags on attachment: 144691 Committed r119008: <http://trac.webkit.org/changeset/119008> All reviewed patches have been landed. Closing bug. |