205889 2020-01-07 14:51:49 -0800 Remove LayoutUnit::operator(const float&) to avoid bad results when the LHS is accidentally made an int 2025-01-06 09:02:12 -0800 1 1 1 Unclassified WebKit WebCore Misc. WebKit Local Build Unspecified Unspecified NEW https://bugs.webkit.org/show_bug.cgi?id=205563 EasyFix, GoodFirstBug, InRadar P2 Normal --- 1 dbates yulun_wu ahmad.saleem792 karlcow webkit-bug-importer zalan oldest_to_newest 1603806 0 dbates 2020-01-07 14:51:49 -0800 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. 2050925 1 ahmad.saleem792 2024-08-06 09:53:01 -0700 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? 2067045 2 ahmad.saleem792 2024-10-10 16:10:25 -0700 (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`. 2067046 3 ahmad.saleem792 2024-10-10 16:10:42 -0700 (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() }; 2085068 4 webkit-bug-importer 2025-01-06 09:02:12 -0800 <rdar://problem/142420106>