[webkit-reviews] review denied: [Bug 54244] Convert the line box tree to floating point and eliminate font rounding hacks : [Attachment 82356] Ready for review!

[bugzilla-daemon at webkit.org]

mitz at webkit.org has denied Dave Hyatt <hyatt at apple.com>'s request for review:
Bug 54244: Convert the line box tree to floating point and eliminate font
rounding hacks
https://bugs.webkit.org/show_bug.cgi?id=54244

Attachment 82356: Ready for review!
https://bugs.webkit.org/attachment.cgi?id=82356&action=review

------- Additional Comments from mitz at webkit.org
View in context: https://bugs.webkit.org/attachment.cgi?id=82356&action=review

r- because of EWS build failures. Some minor comments inside.

> Source/WebCore/ChangeLog:8
> +	   hacks from the Font code and makes sure all Font APIS involving
width measurement and width offsets use floats.

Typo: APIS

> Source/WebCore/platform/graphics/WidthIterator.cpp:173
> +			   float expansion = previousExpansion - m_expansion;

Don’t need this anymore, can just use m_expansionPerOpportunity. You don’t need
previousExpansion at all I think.

> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:480
> +			       float expansion = previousExpansion -
m_expansion;

Here too you can get rid of expansion and previousExpansion.

> Source/WebCore/platform/graphics/win/UniscribeController.cpp:-323
> -	   // Match AppKit's rules for the integer vs. non-integer rendering
modes.
> -	   float roundedAdvance = roundf(advance);
> -	   if (!m_font.isPrinterFont() && !fontData->isSystemFont()) {
> -	       advance = roundedAdvance;
> -	       offsetX = roundf(offsetX);
> -	       offsetY = roundf(offsetY);
> -	   }

Don’t we need this still?

> Source/WebCore/platform/graphics/win/UniscribeController.cpp:319
>			   float previousPadding = m_padding;
>			   m_padding -= m_padPerSpace;
> -			   advance += roundf(previousPadding) -
roundf(m_padding);
> +			   advance += previousPadding - m_padding;

Again, no need for previousPadding anymore. Just add m_padPerSpace to advance
and subtract m_padPerSpace from m_padding.

> Source/WebCore/rendering/InlineBox.cpp:109
> + 

Space!

> Source/WebCore/rendering/InlineBox.h:312
> +    

Spaces!

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:397
> +		       trailingSpaceRun->m_box->setLogicalWidth(max(0.f,
trailingSpaceRun->m_box->logicalWidth() - totalLogicalWidth +
availableLogicalWidth));

Another way to write this is max<float>(0, …)

> Source/WebCore/rendering/RenderText.cpp:509
> +    // Go ahead and round left to snap it to the nearest pixel.

:)

> Source/WebCore/rendering/RenderText.cpp:705
> +	   const_cast<RenderText*>(this)->computePreferredLogicalWidths(0.f);

Is this .f needed?

> Source/WebCore/rendering/RenderText.cpp:713
> +	   const_cast<RenderText*>(this)->computePreferredLogicalWidths(0.f);

Ditto.

> Source/WebCore/rendering/RenderTreeAsText.cpp:474
> +    // to detect any changes caused by the conversion to floating point. :(

The best thing you can do is generate pixel results for all tests on your
machine w/o the patch, then apply the patch and run-webkit-tests --pixel and
see if there are any significant differences.