33189 2010-01-04 17:01:14 -0800 scaleEmToUnits in SimpleFontDataMac.mm does an unnecessary division 2010-01-04 19:56:53 -0800 1 1 1 Unclassified WebKit Layout and Rendering 528+ (Nightly build) PC OS X 10.5 RESOLVED FIXED P2 Normal --- 1 ojan webkit-unassigned webkit.review.bot oldest_to_newest 176945 0 ojan 2010-01-04 17:01:14 -0800 It's not a big deal, but I stumbled upon it and thought I'd clean it up. const float contextDPI = 72.0f; static inline float scaleEmToUnits(float x, unsigned unitsPerEm) { return x * (contextDPI / (contextDPI * unitsPerEm)); } can just be static inline float scaleEmToUnits(float x, unsigned unitsPerEm) { return x / static_cast<float>(unitsPerEm); } Technically, there could be minor differences here due to float rounding I think, but in practice I think this is just an accident of history. The code was added here http://trac.webkit.org/changeset/8104. I assume this was done to match the previously private function, but we don't use that function anymore. In either case, the layout tests pass with this change. 176966 1 45847 ojan 2010-01-04 17:36:36 -0800 Created attachment 45847 2010-01-04 Nikolas Zimmermann <nzimmermann@rim.com> 176968 2 45847 darin 2010-01-04 17:37:23 -0800 Comment on attachment 45847 2010-01-04 Nikolas Zimmermann <nzimmermann@rim.com> Looks like this is the wrong patch. 176971 3 45847 ojan 2010-01-04 17:38:42 -0800 Comment on attachment 45847 2010-01-04 Nikolas Zimmermann <nzimmermann@rim.com> Holy cow that was fast. I was just marking this patch obsolete! Bugzilla-tool post-commits is confusing. head~1 really should be the top commit, not the second one. Sorry for the spam. 176972 4 45849 ojan 2010-01-04 17:39:31 -0800 Created attachment 45849 Remove unnecessary division 176976 5 45849 darin 2010-01-04 18:01:48 -0800 Comment on attachment 45849 Remove unnecessary division > -static inline float scaleEmToUnits(float x, unsigned unitsPerEm) { return x * (contextDPI / (contextDPI * unitsPerEm)); } > +static inline float scaleEmToUnits(float x, unsigned unitsPerEm) { return x / static_cast<float>(unitsPerEm); } Why the static_cast<float>? As far as I know, C will do a float / unsigned just fine, and return a float without any cast. 176978 6 webkit.review.bot 2010-01-04 18:04:49 -0800 style-queue ran check-webkit-style on attachment 45849 without any errors. 176984 7 45854 ojan 2010-01-04 18:12:09 -0800 Created attachment 45854 Patch without static_cast. 176986 8 ojan 2010-01-04 18:13:11 -0800 (In reply to comment #5) > (From update of attachment 45849 [details]) > > -static inline float scaleEmToUnits(float x, unsigned unitsPerEm) { return x * (contextDPI / (contextDPI * unitsPerEm)); } > > +static inline float scaleEmToUnits(float x, unsigned unitsPerEm) { return x / static_cast<float>(unitsPerEm); } > > Why the static_cast<float>? As far as I know, C will do a float / unsigned just > fine, and return a float without any cast. I was just mimicking code used in the other platform's versions of scaleEmToUnits. Removed. 176991 9 45856 ojan 2010-01-04 18:50:23 -0800 Created attachment 45856 Patch without static_cast for real this time. 176994 10 45856 mitz 2010-01-04 18:58:47 -0800 Comment on attachment 45856 Patch without static_cast for real this time. > + Need a short description and bug URL (OOPS!) Please remove that line. > + No new tests. Covered by existing tests. More like this code change does not change behavior. 177001 11 ojan 2010-01-04 19:56:53 -0800 http://trac.webkit.org/changeset/52782 45847 2010-01-04 17:36:36 -0800 2010-01-04 17:38:42 -0800 2010-01-04 Nikolas Zimmermann <nzimmermann@rim.com> bug-33189-20100104173635.patch text/plain 1075 ojan ZGlmZiAtLWdpdCBhL1dlYkNvcmUvQ2hhbmdlTG9nIGIvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXgg YzdiMGVjYy4uMTdlZTgxNSAxMDA2NDQKLS0tIGEvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvV2Vi Q29yZS9DaGFuZ2VMb2cKQEAgLTEsNSArMSwxMSBAQAogMjAxMC0wMS0wNCAgTmlrb2xhcyBaaW1t ZXJtYW5uICA8bnppbW1lcm1hbm5AcmltLmNvbT4KIAorICAgICAgICBOb3QgcmV2aWV3ZWQuIEZv cmdvdCB0byBhZGQgU1ZHTWFya2VyRGF0YS5oIC8gU1ZHTWFya2VyTGF5b3V0SW5mby4oY3BwLGgp IHRvIENocm9taXVtIGJ1aWxkLgorCisgICAgICAgICogV2ViQ29yZS5neXBpOgorCisyMDEwLTAx LTA0ICBOaWtvbGFzIFppbW1lcm1hbm4gIDxuemltbWVybWFubkByaW0uY29tPgorCiAgICAgICAg IFJldmlld2VkIGJ5IERpcmsgU2NodWx6ZS4KIAogICAgICAgICBNYXJrZXIgY29kZSBpcyBidWdn eTogcmVmZXJlbmNlUG9pbnQgdHJhbnNsYXRpb24gaXMgb2ZmCmRpZmYgLS1naXQgYS9XZWJDb3Jl L1dlYkNvcmUuZ3lwaSBiL1dlYkNvcmUvV2ViQ29yZS5neXBpCmluZGV4IGEzMzY2OTMuLjgxOWMw ODIgMTAwNjQ0Ci0tLSBhL1dlYkNvcmUvV2ViQ29yZS5neXBpCisrKyBiL1dlYkNvcmUvV2ViQ29y ZS5neXBpCkBAIC0zMTMxLDYgKzMxMzEsOSBAQAogICAgICAgICAgICAgJ3JlbmRlcmluZy9TVkdJ bmxpbmVGbG93Qm94LmgnLAogICAgICAgICAgICAgJ3JlbmRlcmluZy9TVkdJbmxpbmVUZXh0Qm94 LmNwcCcsCiAgICAgICAgICAgICAncmVuZGVyaW5nL1NWR0lubGluZVRleHRCb3guaCcsCisgICAg ICAgICAgICAncmVuZGVyaW5nL1NWR01hcmtlckRhdGEuaCcsCisgICAgICAgICAgICAncmVuZGVy aW5nL1NWR01hcmtlckxheW91dEluZm8uY3BwJywKKyAgICAgICAgICAgICdyZW5kZXJpbmcvU1ZH TWFya2VyTGF5b3V0SW5mby5oJywKICAgICAgICAgICAgICdyZW5kZXJpbmcvU1ZHUmVuZGVyU3Vw cG9ydC5jcHAnLAogICAgICAgICAgICAgJ3JlbmRlcmluZy9TVkdSZW5kZXJTdXBwb3J0LmgnLAog ICAgICAgICAgICAgJ3JlbmRlcmluZy9TVkdSZW5kZXJUcmVlQXNUZXh0LmNwcCcsCg== 45849 2010-01-04 17:39:31 -0800 2010-01-04 18:12:05 -0800 Remove unnecessary division bug-33189-20100104173929.patch text/plain 1590 ojan ZGlmZiAtLWdpdCBhL1dlYkNvcmUvQ2hhbmdlTG9nIGIvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXgg MTdlZTgxNS4uZDYzMjBjNyAxMDA2NDQKLS0tIGEvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvV2Vi Q29yZS9DaGFuZ2VMb2cKQEAgLTEsMyArMSwyMCBAQAorMjAxMC0wMS0wNCAgT2phbiBWYWZhaSAg PG9qYW5Ab2phbm1hY3Byby5zZm8uY29ycC5nb29nbGUuY29tPgorCisgICAgICAgIFJldmlld2Vk IGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFJlbW92ZSB1bm5lY2Vzc2FyeSBkaXZpc2lv biBieSBhIGNvbnN0YW50IGluIFNpbXBsZUZvbnREYXRhTWFjLm1tLgorICAgICAgICBodHRwczov L2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MzMxODkKKworICAgICAgICBUaGlzIHdh cyBhZGRlZCBpbiBodHRwOi8vdHJhYy53ZWJraXQub3JnL2NoYW5nZXNldC84MTA0IGFuZCBzZWVt cyBsaWtlCisgICAgICAgIGl0IHdhcyBqdXN0IHRoZXJlIHRvIG1hdGNoIHRoZSBwcmV2aW91c2x5 IHByaXZhdGUgZnVuY3Rpb24gd2hpY2ggd2Ugbm8KKyAgICAgICAgbG9uZ2VyIHVzZS4KKworICAg ICAgICBOZWVkIGEgc2hvcnQgZGVzY3JpcHRpb24gYW5kIGJ1ZyBVUkwgKE9PUFMhKQorCisgICAg ICAgIE5vIG5ldyB0ZXN0cy4gQ292ZXJlZCBieSBleGlzdGluZyB0ZXN0cy4KKworICAgICAgICAq IHBsYXRmb3JtL2dyYXBoaWNzL21hYy9TaW1wbGVGb250RGF0YU1hYy5tbToKKwogMjAxMC0wMS0w NCAgTmlrb2xhcyBaaW1tZXJtYW5uICA8bnppbW1lcm1hbm5AcmltLmNvbT4KIAogICAgICAgICBO b3QgcmV2aWV3ZWQuIEZvcmdvdCB0byBhZGQgU1ZHTWFya2VyRGF0YS5oIC8gU1ZHTWFya2VyTGF5 b3V0SW5mby4oY3BwLGgpIHRvIENocm9taXVtIGJ1aWxkLgpkaWZmIC0tZ2l0IGEvV2ViQ29yZS9w bGF0Zm9ybS9ncmFwaGljcy9tYWMvU2ltcGxlRm9udERhdGFNYWMubW0gYi9XZWJDb3JlL3BsYXRm b3JtL2dyYXBoaWNzL21hYy9TaW1wbGVGb250RGF0YU1hYy5tbQppbmRleCA5N2E3MjUxLi43YzRi YmIwIDEwMDY0NAotLS0gYS9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL21hYy9TaW1wbGVGb250 RGF0YU1hYy5tbQorKysgYi9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL21hYy9TaW1wbGVGb250 RGF0YU1hYy5tbQpAQCAtNTUsOCArNTUsNyBAQCB1c2luZyBuYW1lc3BhY2Ugc3RkOwogbmFtZXNw YWNlIFdlYkNvcmUgewogICAKIGNvbnN0IGZsb2F0IHNtYWxsQ2Fwc0ZvbnRTaXplTXVsdGlwbGll ciA9IDAuN2Y7Ci1jb25zdCBmbG9hdCBjb250ZXh0RFBJID0gNzIuMGY7Ci1zdGF0aWMgaW5saW5l IGZsb2F0IHNjYWxlRW1Ub1VuaXRzKGZsb2F0IHgsIHVuc2lnbmVkIHVuaXRzUGVyRW0pIHsgcmV0 dXJuIHggKiAoY29udGV4dERQSSAvIChjb250ZXh0RFBJICogdW5pdHNQZXJFbSkpOyB9CitzdGF0 aWMgaW5saW5lIGZsb2F0IHNjYWxlRW1Ub1VuaXRzKGZsb2F0IHgsIHVuc2lnbmVkIHVuaXRzUGVy RW0pIHsgcmV0dXJuIHggLyBzdGF0aWNfY2FzdDxmbG9hdD4odW5pdHNQZXJFbSk7IH0KIAogc3Rh dGljIGJvb2wgaW5pdEZvbnREYXRhKFNpbXBsZUZvbnREYXRhKiBmb250RGF0YSkKIHsK 45854 2010-01-04 18:12:09 -0800 2010-01-04 18:50:16 -0800 Patch without static_cast. bug-33189-20100104181208.patch text/plain 1590 ojan ZGlmZiAtLWdpdCBhL1dlYkNvcmUvQ2hhbmdlTG9nIGIvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXgg MTdlZTgxNS4uZDYzMjBjNyAxMDA2NDQKLS0tIGEvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvV2Vi Q29yZS9DaGFuZ2VMb2cKQEAgLTEsMyArMSwyMCBAQAorMjAxMC0wMS0wNCAgT2phbiBWYWZhaSAg PG9qYW5Ab2phbm1hY3Byby5zZm8uY29ycC5nb29nbGUuY29tPgorCisgICAgICAgIFJldmlld2Vk IGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFJlbW92ZSB1bm5lY2Vzc2FyeSBkaXZpc2lv biBieSBhIGNvbnN0YW50IGluIFNpbXBsZUZvbnREYXRhTWFjLm1tLgorICAgICAgICBodHRwczov L2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MzMxODkKKworICAgICAgICBUaGlzIHdh cyBhZGRlZCBpbiBodHRwOi8vdHJhYy53ZWJraXQub3JnL2NoYW5nZXNldC84MTA0IGFuZCBzZWVt cyBsaWtlCisgICAgICAgIGl0IHdhcyBqdXN0IHRoZXJlIHRvIG1hdGNoIHRoZSBwcmV2aW91c2x5 IHByaXZhdGUgZnVuY3Rpb24gd2hpY2ggd2Ugbm8KKyAgICAgICAgbG9uZ2VyIHVzZS4KKworICAg ICAgICBOZWVkIGEgc2hvcnQgZGVzY3JpcHRpb24gYW5kIGJ1ZyBVUkwgKE9PUFMhKQorCisgICAg ICAgIE5vIG5ldyB0ZXN0cy4gQ292ZXJlZCBieSBleGlzdGluZyB0ZXN0cy4KKworICAgICAgICAq IHBsYXRmb3JtL2dyYXBoaWNzL21hYy9TaW1wbGVGb250RGF0YU1hYy5tbToKKwogMjAxMC0wMS0w NCAgTmlrb2xhcyBaaW1tZXJtYW5uICA8bnppbW1lcm1hbm5AcmltLmNvbT4KIAogICAgICAgICBO b3QgcmV2aWV3ZWQuIEZvcmdvdCB0byBhZGQgU1ZHTWFya2VyRGF0YS5oIC8gU1ZHTWFya2VyTGF5 b3V0SW5mby4oY3BwLGgpIHRvIENocm9taXVtIGJ1aWxkLgpkaWZmIC0tZ2l0IGEvV2ViQ29yZS9w bGF0Zm9ybS9ncmFwaGljcy9tYWMvU2ltcGxlRm9udERhdGFNYWMubW0gYi9XZWJDb3JlL3BsYXRm b3JtL2dyYXBoaWNzL21hYy9TaW1wbGVGb250RGF0YU1hYy5tbQppbmRleCA5N2E3MjUxLi43YzRi YmIwIDEwMDY0NAotLS0gYS9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL21hYy9TaW1wbGVGb250 RGF0YU1hYy5tbQorKysgYi9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL21hYy9TaW1wbGVGb250 RGF0YU1hYy5tbQpAQCAtNTUsOCArNTUsNyBAQCB1c2luZyBuYW1lc3BhY2Ugc3RkOwogbmFtZXNw YWNlIFdlYkNvcmUgewogICAKIGNvbnN0IGZsb2F0IHNtYWxsQ2Fwc0ZvbnRTaXplTXVsdGlwbGll ciA9IDAuN2Y7Ci1jb25zdCBmbG9hdCBjb250ZXh0RFBJID0gNzIuMGY7Ci1zdGF0aWMgaW5saW5l IGZsb2F0IHNjYWxlRW1Ub1VuaXRzKGZsb2F0IHgsIHVuc2lnbmVkIHVuaXRzUGVyRW0pIHsgcmV0 dXJuIHggKiAoY29udGV4dERQSSAvIChjb250ZXh0RFBJICogdW5pdHNQZXJFbSkpOyB9CitzdGF0 aWMgaW5saW5lIGZsb2F0IHNjYWxlRW1Ub1VuaXRzKGZsb2F0IHgsIHVuc2lnbmVkIHVuaXRzUGVy RW0pIHsgcmV0dXJuIHggLyBzdGF0aWNfY2FzdDxmbG9hdD4odW5pdHNQZXJFbSk7IH0KIAogc3Rh dGljIGJvb2wgaW5pdEZvbnREYXRhKFNpbXBsZUZvbnREYXRhKiBmb250RGF0YSkKIHsK 45856 2010-01-04 18:50:23 -0800 2010-01-04 18:58:47 -0800 Patch without static_cast for real this time. bug-33189-20100104185021.patch text/plain 1570 ojan ZGlmZiAtLWdpdCBhL1dlYkNvcmUvQ2hhbmdlTG9nIGIvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXgg MTdlZTgxNS4uZDYzMjBjNyAxMDA2NDQKLS0tIGEvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvV2Vi Q29yZS9DaGFuZ2VMb2cKQEAgLTEsMyArMSwyMCBAQAorMjAxMC0wMS0wNCAgT2phbiBWYWZhaSAg PG9qYW5Ab2phbm1hY3Byby5zZm8uY29ycC5nb29nbGUuY29tPgorCisgICAgICAgIFJldmlld2Vk IGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFJlbW92ZSB1bm5lY2Vzc2FyeSBkaXZpc2lv biBieSBhIGNvbnN0YW50IGluIFNpbXBsZUZvbnREYXRhTWFjLm1tLgorICAgICAgICBodHRwczov L2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MzMxODkKKworICAgICAgICBUaGlzIHdh cyBhZGRlZCBpbiBodHRwOi8vdHJhYy53ZWJraXQub3JnL2NoYW5nZXNldC84MTA0IGFuZCBzZWVt cyBsaWtlCisgICAgICAgIGl0IHdhcyBqdXN0IHRoZXJlIHRvIG1hdGNoIHRoZSBwcmV2aW91c2x5 IHByaXZhdGUgZnVuY3Rpb24gd2hpY2ggd2Ugbm8KKyAgICAgICAgbG9uZ2VyIHVzZS4KKworICAg ICAgICBOZWVkIGEgc2hvcnQgZGVzY3JpcHRpb24gYW5kIGJ1ZyBVUkwgKE9PUFMhKQorCisgICAg ICAgIE5vIG5ldyB0ZXN0cy4gQ292ZXJlZCBieSBleGlzdGluZyB0ZXN0cy4KKworICAgICAgICAq IHBsYXRmb3JtL2dyYXBoaWNzL21hYy9TaW1wbGVGb250RGF0YU1hYy5tbToKKwogMjAxMC0wMS0w NCAgTmlrb2xhcyBaaW1tZXJtYW5uICA8bnppbW1lcm1hbm5AcmltLmNvbT4KIAogICAgICAgICBO b3QgcmV2aWV3ZWQuIEZvcmdvdCB0byBhZGQgU1ZHTWFya2VyRGF0YS5oIC8gU1ZHTWFya2VyTGF5 b3V0SW5mby4oY3BwLGgpIHRvIENocm9taXVtIGJ1aWxkLgpkaWZmIC0tZ2l0IGEvV2ViQ29yZS9w bGF0Zm9ybS9ncmFwaGljcy9tYWMvU2ltcGxlRm9udERhdGFNYWMubW0gYi9XZWJDb3JlL3BsYXRm b3JtL2dyYXBoaWNzL21hYy9TaW1wbGVGb250RGF0YU1hYy5tbQppbmRleCA5N2E3MjUxLi5jNDZi YTFjIDEwMDY0NAotLS0gYS9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL21hYy9TaW1wbGVGb250 RGF0YU1hYy5tbQorKysgYi9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL21hYy9TaW1wbGVGb250 RGF0YU1hYy5tbQpAQCAtNTUsOCArNTUsNyBAQCB1c2luZyBuYW1lc3BhY2Ugc3RkOwogbmFtZXNw YWNlIFdlYkNvcmUgewogICAKIGNvbnN0IGZsb2F0IHNtYWxsQ2Fwc0ZvbnRTaXplTXVsdGlwbGll ciA9IDAuN2Y7Ci1jb25zdCBmbG9hdCBjb250ZXh0RFBJID0gNzIuMGY7Ci1zdGF0aWMgaW5saW5l IGZsb2F0IHNjYWxlRW1Ub1VuaXRzKGZsb2F0IHgsIHVuc2lnbmVkIHVuaXRzUGVyRW0pIHsgcmV0 dXJuIHggKiAoY29udGV4dERQSSAvIChjb250ZXh0RFBJICogdW5pdHNQZXJFbSkpOyB9CitzdGF0 aWMgaW5saW5lIGZsb2F0IHNjYWxlRW1Ub1VuaXRzKGZsb2F0IHgsIHVuc2lnbmVkIHVuaXRzUGVy RW0pIHsgcmV0dXJuIHggLyB1bml0c1BlckVtOyB9CiAKIHN0YXRpYyBib29sIGluaXRGb250RGF0 YShTaW1wbGVGb250RGF0YSogZm9udERhdGEpCiB7Cg==