Bug 33189 – scaleEmToUnits in SimpleFontDataMac.mm does an unnecessary division

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Ojan Vafai]

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.

[Ojan Vafai]

Created attachment 45847 [details]
2010-01-04  Nikolas Zimmermann  <[e-mail]>

[Darin Adler]

Comment on attachment 45847 [details]
2010-01-04  Nikolas Zimmermann  <[e-mail]>

Looks like this is the wrong patch.

[Ojan Vafai]

Comment on attachment 45847 [details]
2010-01-04  Nikolas Zimmermann  <[e-mail]>

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.

[Ojan Vafai]

Created attachment 45849 [details]
Remove unnecessary division

[Darin Adler]

Comment on attachment 45849 [details]
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.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45849 [details] without any errors.

[Ojan Vafai]

Created attachment 45854 [details]
Patch without static_cast.

[Ojan Vafai]

(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.

[Ojan Vafai]

Created attachment 45856 [details]
Patch without static_cast for real this time.

Comment on attachment 45856 [details]
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.

[Ojan Vafai]

http://trac.webkit.org/changeset/52782