122367 2013-10-04 18:04:31 -0700 [MathML] Use of floating point floor/ceil operations on LayoutUnits seems wrong 2013-10-08 13:41:18 -0700 1 1 1 Unclassified WebKit MathML 528+ (Nightly build) All All RESOLVED FIXED InRadar P2 Normal --- 121416 1 bfulgham bfulgham bfulgham commit-queue dbarton esprehn+autocc fred.wang glenn hyatt kondapallykalyan mrobinson webkit-bug-importer oldest_to_newest 936463 0 bfulgham 2013-10-04 18:04:31 -0700 While investigating another problem, I noticed the following code in RenderMathMLOperator::paintCharacter: switch (trim) { case TrimTop: glyphPaintRect.shiftYEdgeTo(ceil(glyphPaintRect.y()) + 1); clipBounds.shiftYEdgeTo(glyphPaintRect.y()); break; case TrimBottom: glyphPaintRect.shiftMaxYEdgeTo(floor(glyphPaintRect.maxY()) - 1); clipBounds.shiftMaxYEdgeTo(glyphPaintRect.maxY()); break; case TrimTopAndBottom: glyphPaintRect.shiftYEdgeTo(ceil(glyphPaintRect.y() + 1)); glyphPaintRect.shiftMaxYEdgeTo(floor(glyphPaintRect.maxY()) - 1); clipBounds.shiftYEdgeTo(glyphPaintRect.y()); clipBounds.shiftMaxYEdgeTo(glyphPaintRect.maxY()); break; } The various paint rects in this code are LayoutRects, and the y()/maxY() methods return LayoutUnits. The LayoutRect and LayoutUnit wrap an 'int' type. So the calls to the ceil/floor C API results in an implicit cast from LayoutUnit->float, then a call to floor/ceil. Since we are already working with integer types, these calls seem at best wasteful. Given that the layout units do some magic internally for dealing with sub-pixel layout for their ceil() and floor() methods, I feel like this code should be revised as outlined in the attached patch. 936467 1 213428 bfulgham 2013-10-04 18:08:26 -0700 Created attachment 213428 Patch 936486 2 mrobinson 2013-10-04 19:47:06 -0700 landing I would like to verify that this fits not regress rendering quality on GTK+, if you don't mind. 937325 3 mrobinson 2013-10-07 19:13:37 -0700 This looks good on GTK+ so feel free to land this when it looks like the the patch for 12146 sticks. 937545 4 webkit-bug-importer 2013-10-08 09:25:56 -0700 <rdar://problem/15176294> 937658 5 bfulgham 2013-10-08 13:41:18 -0700 Committed r157135: <http://trac.webkit.org/changeset/157135> 213428 2013-10-04 18:08:26 -0700 2013-10-04 18:44:39 -0700 Patch bug-122367-20131004180825.patch text/plain 2148 bfulgham SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDE1NjkzOCkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE2IEBACisyMDEzLTEwLTA0ICBCcmVudCBG dWxnaGFtICA8YmZ1bGdoYW1AYXBwbGUuY29tPgorCisgICAgICAgIFtNYXRoTUxdIFVzZSBvZiBm bG9hdGluZyBwb2ludCBmbG9vci9jZWlsIG9uIExheW91dFVuaXRzIHNlZW1zIHdyb25nCisgICAg ICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xMjIzNjcKKworICAg ICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBObyBuZXcgdGVzdHMg KE9PUFMhKS4KKworICAgICAgICAqIHJlbmRlcmluZy9tYXRobWwvUmVuZGVyTWF0aE1MT3BlcmF0 b3IuY3BwOgorICAgICAgICAoV2ViQ29yZTo6UmVuZGVyTWF0aE1MT3BlcmF0b3I6OnBhaW50Q2hh cmFjdGVyKTogVXNlIG1lbWJlciBtZXRob2RzIHRvIGNvbXB1dGUKKyAgICAgICAgY29ycmVjdCBm bG9vci9jZWlsIHZhbHVlcyBmb3IgTGF5b3V0VW5pdHMuCisKIDIwMTMtMTAtMDQgIEFuZHJlYXMg S2xpbmcgIDxha2xpbmdAYXBwbGUuY29tPgogCiAgICAgICAgIEVkaXRvciBzaG91bGQgdXNlIHJl ZmVyZW5jZS1nZXR0aW5nIGRvY3VtZW50KCkgaW50ZXJuYWxseS4KSW5kZXg6IFNvdXJjZS9XZWJD b3JlL3JlbmRlcmluZy9tYXRobWwvUmVuZGVyTWF0aE1MT3BlcmF0b3IuY3BwCj09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0K LS0tIFNvdXJjZS9XZWJDb3JlL3JlbmRlcmluZy9tYXRobWwvUmVuZGVyTWF0aE1MT3BlcmF0b3Iu Y3BwCShyZXZpc2lvbiAxNTY5MzgpCisrKyBTb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvbWF0aG1s L1JlbmRlck1hdGhNTE9wZXJhdG9yLmNwcAkod29ya2luZyBjb3B5KQpAQCAtMjk4LDE2ICsyOTgs MTcgQEAgTGF5b3V0UmVjdCBSZW5kZXJNYXRoTUxPcGVyYXRvcjo6cGFpbnRDaAogICAgIEZsb2F0 UmVjdCBjbGlwQm91bmRzID0gaW5mby5yZWN0OwogICAgIHN3aXRjaCAodHJpbSkgewogICAgIGNh c2UgVHJpbVRvcDoKLSAgICAgICAgZ2x5cGhQYWludFJlY3Quc2hpZnRZRWRnZVRvKGNlaWxmKGds eXBoUGFpbnRSZWN0LnkoKSkgKyAxKTsKKyAgICAgICAgZ2x5cGhQYWludFJlY3Quc2hpZnRZRWRn ZVRvKGdseXBoUGFpbnRSZWN0LnkoKS5jZWlsKCkgKyAxKTsKICAgICAgICAgY2xpcEJvdW5kcy5z aGlmdFlFZGdlVG8oZ2x5cGhQYWludFJlY3QueSgpKTsKICAgICAgICAgYnJlYWs7CiAgICAgY2Fz ZSBUcmltQm90dG9tOgotICAgICAgICBnbHlwaFBhaW50UmVjdC5zaGlmdE1heFlFZGdlVG8oZmxv b3JmKGdseXBoUGFpbnRSZWN0Lm1heFkoKSkgLSAxKTsKKyAgICAgICAgZ2x5cGhQYWludFJlY3Qu c2hpZnRNYXhZRWRnZVRvKGdseXBoUGFpbnRSZWN0Lm1heFkoKS5mbG9vcigpIC0gMSk7CiAgICAg ICAgIGNsaXBCb3VuZHMuc2hpZnRNYXhZRWRnZVRvKGdseXBoUGFpbnRSZWN0Lm1heFkoKSk7CiAg ICAgICAgIGJyZWFrOwogICAgIGNhc2UgVHJpbVRvcEFuZEJvdHRvbToKLSAgICAgICAgZ2x5cGhQ YWludFJlY3Quc2hpZnRZRWRnZVRvKGNlaWxmKGdseXBoUGFpbnRSZWN0LnkoKSArIDEpKTsKLSAg ICAgICAgZ2x5cGhQYWludFJlY3Quc2hpZnRNYXhZRWRnZVRvKGZsb29yZihnbHlwaFBhaW50UmVj dC5tYXhZKCkpIC0gMSk7CisgICAgICAgIExheW91dFVuaXQgdGVtcCA9IGdseXBoUGFpbnRSZWN0 LnkoKSArIDE7CisgICAgICAgIGdseXBoUGFpbnRSZWN0LnNoaWZ0WUVkZ2VUbyh0ZW1wLmNlaWwo KSk7CisgICAgICAgIGdseXBoUGFpbnRSZWN0LnNoaWZ0TWF4WUVkZ2VUbyhnbHlwaFBhaW50UmVj dC5tYXhZKCkuZmxvb3IoKSAtIDEpOwogICAgICAgICBjbGlwQm91bmRzLnNoaWZ0WUVkZ2VUbyhn bHlwaFBhaW50UmVjdC55KCkpOwogICAgICAgICBjbGlwQm91bmRzLnNoaWZ0TWF4WUVkZ2VUbyhn bHlwaFBhaW50UmVjdC5tYXhZKCkpOwogICAgICAgICBicmVhazsK