Bug 205889 – Remove LayoutUnit::operator(const float&) to avoid bad results when the LHS is accidentally made an int

• Status: NEW
• Component: WebCore Misc.

[Daniel Bates]

In the patch for bug #205563 I moved some code:

[[
...
LayoutUnit logicalWidth = snappedSelectionRect.width();
if (snappedSelectionRect.x() > logicalRight())
    logicalWidth = 0;
else if (snappedSelectionRect.maxX() > logicalRight())
    logicalWidth = logicalRight() - snappedSelectionRect.x();
...
]]

And in the process changed:

LayoutUnit logicalWidth = snappedSelectionRect.width();

to

auto logicalWidth = snappedSelectionRect.width();

As a result logicalWidth was now an int. This led to bad things when "snappedSelectionRect.maxX() > logicalRight()" evaluates to true because we would then compute "logicalRight() - snappedSelectionRect.x()" (which would be a float since logicalRight() is a float) and then integer truncate to assign to logicalWidth.

This bug would have been prevented if LayoutUnit::operator(const float&) did not exist because it would have forced the code to have been written (pre-my move):

[[
...
LayoutUnit logicalWidth = snappedSelectionRect.width();
if (snappedSelectionRect.x() > logicalRight())
    logicalWidth = 0;
else if (snappedSelectionRect.maxX() > logicalRight())
    logicalWidth = LayoutUnit { logicalRight() - snappedSelectionRect.x() };
...
]]

(Notice the explicit constructor call to LayoutUnit in the second branch).

And so if I had then changed LayoutUnit => auto it would have compile-time failed and I would have seen my mistake because the code would now be trying to illogically assign a LayoutUnit to an int.

[Ahmad Saleem]

https://searchfox.org/wubkat/rev/b36cbce69fddb7da33823f316bd8ead5bebee970/Source/WebCore/rendering/LegacyInlineTextBox.cpp#164

LayoutRect snappedSelectionRect(const LayoutRect& selectionRect, float logicalRight, float selectionTop, float selectionHeight, bool isHorizontal)
{
    auto snappedSelectionRect = enclosingIntRect(selectionRect);
    LayoutUnit logicalWidth = snappedSelectionRect.width();
    if (snappedSelectionRect.x() > logicalRight)
        logicalWidth = 0;
    else if (snappedSelectionRect.maxX() > logicalRight)
        logicalWidth = logicalRight - snappedSelectionRect.x();

    LayoutPoint topPoint;
    LayoutUnit width;
    LayoutUnit height;
    if (isHorizontal) {
        topPoint = LayoutPoint { snappedSelectionRect.x(), selectionTop };
        width = logicalWidth;
        height = selectionHeight;
    } else {
        topPoint = LayoutPoint { selectionTop, snappedSelectionRect.x() };
        width = selectionHeight;
        height = logicalWidth;
    }
    return LayoutRect { topPoint, LayoutSize { width, height } };
}

____

@Alan - it is in `LegacyInline` layout - do we need to do anything here?

[Ahmad Saleem]

(In reply to Ahmad Saleem from comment #1)
> https://searchfox.org/wubkat/rev/b36cbce69fddb7da33823f316bd8ead5bebee970/
> Source/WebCore/rendering/LegacyInlineTextBox.cpp#164
> 
> LayoutRect snappedSelectionRect(const LayoutRect& selectionRect, float
> logicalRight, float selectionTop, float selectionHeight, bool isHorizontal)
> {
>     auto snappedSelectionRect = enclosingIntRect(selectionRect);
>     LayoutUnit logicalWidth = snappedSelectionRect.width();
>     if (snappedSelectionRect.x() > logicalRight)
>         logicalWidth = 0;
>     else if (snappedSelectionRect.maxX() > logicalRight)
>         logicalWidth = logicalRight - snappedSelectionRect.x();
> 
>     LayoutPoint topPoint;
>     LayoutUnit width;
>     LayoutUnit height;
>     if (isHorizontal) {
>         topPoint = LayoutPoint { snappedSelectionRect.x(), selectionTop };
>         width = logicalWidth;
>         height = selectionHeight;
>     } else {
>         topPoint = LayoutPoint { selectionTop, snappedSelectionRect.x() };
>         width = selectionHeight;
>         height = logicalWidth;
>     }
>     return LayoutRect { topPoint, LayoutSize { width, height } };
> }
> 
> ____
> 
> @Alan - it is in `LegacyInline` layout - do we need to do anything here?

Just updating:

logicalWidth = logicalRight - snappedSelectionRect.x();

to 

logicalWidth = LayoutUnit { logicalRight() - snappedSelectionRect.x() };

works.. with `build-webkit --release`.

[Ahmad Saleem]

(In reply to Ahmad Saleem from comment #2)
> (In reply to Ahmad Saleem from comment #1)
> > https://searchfox.org/wubkat/rev/b36cbce69fddb7da33823f316bd8ead5bebee970/
> > Source/WebCore/rendering/LegacyInlineTextBox.cpp#164
> > 
> > LayoutRect snappedSelectionRect(const LayoutRect& selectionRect, float
> > logicalRight, float selectionTop, float selectionHeight, bool isHorizontal)
> > {
> >     auto snappedSelectionRect = enclosingIntRect(selectionRect);
> >     LayoutUnit logicalWidth = snappedSelectionRect.width();
> >     if (snappedSelectionRect.x() > logicalRight)
> >         logicalWidth = 0;
> >     else if (snappedSelectionRect.maxX() > logicalRight)
> >         logicalWidth = logicalRight - snappedSelectionRect.x();
> > 
> >     LayoutPoint topPoint;
> >     LayoutUnit width;
> >     LayoutUnit height;
> >     if (isHorizontal) {
> >         topPoint = LayoutPoint { snappedSelectionRect.x(), selectionTop };
> >         width = logicalWidth;
> >         height = selectionHeight;
> >     } else {
> >         topPoint = LayoutPoint { selectionTop, snappedSelectionRect.x() };
> >         width = selectionHeight;
> >         height = logicalWidth;
> >     }
> >     return LayoutRect { topPoint, LayoutSize { width, height } };
> > }
> > 
> > ____
> > 
> > @Alan - it is in `LegacyInline` layout - do we need to do anything here?
> 
> Just updating:
> 
> logicalWidth = logicalRight - snappedSelectionRect.x();
> 
> to 
> 
> logicalWidth = LayoutUnit { logicalRight() - snappedSelectionRect.x() };
> 
> works.. with `build-webkit --release`.

logicalWidth = LayoutUnit { logicalRight - snappedSelectionRect.x() };