Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2294683002:
Simplify contain constraint calculation (Closed)
|
Created:
4 years, 3 months ago by davve Modified:
4 years, 3 months ago Reviewers:
fs CC:
chromium-reviews, krit, rwlbuis, f(malita), blink-reviews, gyuyoung2, Stephen Chennney, kouhei+svg_chromium.org, pdr+svgwatchlist_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSimplify contain constraint calculation
We're selecting between two rectangles given by the intrinsic aspect ratio:
(defaultWidth / aspect ratio, defaultHeight) or (defaultWidth, defaultHeight x
aspect ratio). One rectangle is 'contained' and the other one 'covers'. Select
the 'contained' rectangle by trying out the first to see if its width fit, and
if so we're done. Otherwise compute and select the other rectangle.
Due to how floating point math works, it's possible that both computed
rectangles, or neither computed rectangle, will fit the case when the two ratios
are equal. See https://bugs.chromium.org/p/chromium/issues/detail?id=641221 for
one example. However, in this case it doesn't matter much which rectangle we
choose, the contained and and cover rectangle would be the same rectangle and
the floating point discrepancy has no known practical implication.
BUG=641221
Committed: https://crrev.com/c7b1ab58edd0776579bbf921a7cf905b83883eda
Cr-Commit-Position: refs/heads/master@{#416238}
Patch Set 1 #Patch Set 2 : Rebase and slight re-structure. #Messages
Total messages: 15 (8 generated)
Description was changed from ========== [NOT FOR COMMIT] Simplify contain constraint BUG=641221 ========== to ========== Simplify contain constraint BUG=641221 ==========
Description was changed from ========== Simplify contain constraint BUG=641221 ========== to ========== Simplify contain constraint calculation We're selecting between two rectangles given by the intrinsic aspect ratio: (defaultWidth, defaultHeight x aspect ratio) or (defaultWidth / aspect ratio, defaultHeight). One rectangle is 'contained' and the other one 'covers'. Select the 'contained' rectangle by trying out the first to see if its width fit, and if so we're done. Otherwise compute and select the other rectangle. Due to how floating point math works, it's possible that both computed rectangles, or neither computed rectangle, will fit the case when the two ratios are equal. However, in this case it doesn't matter much which rectangle we choose, the contained and and cover rectangle would be the same rectangle. BUG=641221 ==========
Description was changed from ========== Simplify contain constraint calculation We're selecting between two rectangles given by the intrinsic aspect ratio: (defaultWidth, defaultHeight x aspect ratio) or (defaultWidth / aspect ratio, defaultHeight). One rectangle is 'contained' and the other one 'covers'. Select the 'contained' rectangle by trying out the first to see if its width fit, and if so we're done. Otherwise compute and select the other rectangle. Due to how floating point math works, it's possible that both computed rectangles, or neither computed rectangle, will fit the case when the two ratios are equal. However, in this case it doesn't matter much which rectangle we choose, the contained and and cover rectangle would be the same rectangle. BUG=641221 ========== to ========== Simplify contain constraint calculation We're selecting between two rectangles given by the intrinsic aspect ratio: (defaultWidth, defaultHeight x aspect ratio) or (defaultWidth / aspect ratio, defaultHeight). One rectangle is 'contained' and the other one 'covers'. Select the 'contained' rectangle by trying out the first to see if its width fit, and if so we're done. Otherwise compute and select the other rectangle. Due to how floating point math works, it's possible that both computed rectangles, or neither computed rectangle, will fit the case when the two ratios are equal. However, in this case it doesn't matter much which rectangle we choose, the contained and and cover rectangle would be the same rectangle. BUG=641221 ==========
Description was changed from ========== Simplify contain constraint calculation We're selecting between two rectangles given by the intrinsic aspect ratio: (defaultWidth, defaultHeight x aspect ratio) or (defaultWidth / aspect ratio, defaultHeight). One rectangle is 'contained' and the other one 'covers'. Select the 'contained' rectangle by trying out the first to see if its width fit, and if so we're done. Otherwise compute and select the other rectangle. Due to how floating point math works, it's possible that both computed rectangles, or neither computed rectangle, will fit the case when the two ratios are equal. However, in this case it doesn't matter much which rectangle we choose, the contained and and cover rectangle would be the same rectangle. BUG=641221 ========== to ========== Simplify contain constraint calculation We're selecting between two rectangles given by the intrinsic aspect ratio: (defaultWidth / aspect ratio, defaultHeight) or (defaultWidth, defaultHeight x aspect ratio). One rectangle is 'contained' and the other one 'covers'. Select the 'contained' rectangle by trying out the first to see if its width fit, and if so we're done. Otherwise compute and select the other rectangle. Due to how floating point math works, it's possible that both computed rectangles, or neither computed rectangle, will fit the case when the two ratios are equal. See https://bugs.chromium.org/p/chromium/issues/detail?id=641221 for one example. However, in this case it doesn't matter much which rectangle we choose, the contained and and cover rectangle would be the same rectangle and the floating point discrepancy has no known practical implication. BUG=641221 ==========
davve@opera.com changed reviewers: + fs@opera.com
Alternative solution to BUG=641221 Looking at https://chromium.googlesource.com/chromium/src/+/894b126742579240faefd9ededfb... I see no motivation for the slightly more complex area calculation than: // "... must be assumed to be the largest dimensions..." = easiest answer: the rect with the largest surface area. which I think is a bit confused. The area shouldn't have much to do with rectangle we should choose.
Looking at sizing tests in LayoutTests/svg I think we have most of this covered. Perhaps add a few tests with fractional size and viewBox?
On 2016/09/02 at 09:01:04, davve wrote: > Looking at sizing tests in LayoutTests/svg I think we have most of this covered. Perhaps add a few tests with fractional size and viewBox? That might be good, although I suspect they are more likely to trigger other bugs than this one =) LGTM
On 2016/09/02 09:24:37, fs wrote: > That might be good, although I suspect they are more likely to trigger other > bugs than this one =) That's a good point.
The CQ bit was checked by davve@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Simplify contain constraint calculation We're selecting between two rectangles given by the intrinsic aspect ratio: (defaultWidth / aspect ratio, defaultHeight) or (defaultWidth, defaultHeight x aspect ratio). One rectangle is 'contained' and the other one 'covers'. Select the 'contained' rectangle by trying out the first to see if its width fit, and if so we're done. Otherwise compute and select the other rectangle. Due to how floating point math works, it's possible that both computed rectangles, or neither computed rectangle, will fit the case when the two ratios are equal. See https://bugs.chromium.org/p/chromium/issues/detail?id=641221 for one example. However, in this case it doesn't matter much which rectangle we choose, the contained and and cover rectangle would be the same rectangle and the floating point discrepancy has no known practical implication. BUG=641221 ========== to ========== Simplify contain constraint calculation We're selecting between two rectangles given by the intrinsic aspect ratio: (defaultWidth / aspect ratio, defaultHeight) or (defaultWidth, defaultHeight x aspect ratio). One rectangle is 'contained' and the other one 'covers'. Select the 'contained' rectangle by trying out the first to see if its width fit, and if so we're done. Otherwise compute and select the other rectangle. Due to how floating point math works, it's possible that both computed rectangles, or neither computed rectangle, will fit the case when the two ratios are equal. See https://bugs.chromium.org/p/chromium/issues/detail?id=641221 for one example. However, in this case it doesn't matter much which rectangle we choose, the contained and and cover rectangle would be the same rectangle and the floating point discrepancy has no known practical implication. BUG=641221 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Simplify contain constraint calculation We're selecting between two rectangles given by the intrinsic aspect ratio: (defaultWidth / aspect ratio, defaultHeight) or (defaultWidth, defaultHeight x aspect ratio). One rectangle is 'contained' and the other one 'covers'. Select the 'contained' rectangle by trying out the first to see if its width fit, and if so we're done. Otherwise compute and select the other rectangle. Due to how floating point math works, it's possible that both computed rectangles, or neither computed rectangle, will fit the case when the two ratios are equal. See https://bugs.chromium.org/p/chromium/issues/detail?id=641221 for one example. However, in this case it doesn't matter much which rectangle we choose, the contained and and cover rectangle would be the same rectangle and the floating point discrepancy has no known practical implication. BUG=641221 ========== to ========== Simplify contain constraint calculation We're selecting between two rectangles given by the intrinsic aspect ratio: (defaultWidth / aspect ratio, defaultHeight) or (defaultWidth, defaultHeight x aspect ratio). One rectangle is 'contained' and the other one 'covers'. Select the 'contained' rectangle by trying out the first to see if its width fit, and if so we're done. Otherwise compute and select the other rectangle. Due to how floating point math works, it's possible that both computed rectangles, or neither computed rectangle, will fit the case when the two ratios are equal. See https://bugs.chromium.org/p/chromium/issues/detail?id=641221 for one example. However, in this case it doesn't matter much which rectangle we choose, the contained and and cover rectangle would be the same rectangle and the floating point discrepancy has no known practical implication. BUG=641221 Committed: https://crrev.com/c7b1ab58edd0776579bbf921a7cf905b83883eda Cr-Commit-Position: refs/heads/master@{#416238} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/c7b1ab58edd0776579bbf921a7cf905b83883eda Cr-Commit-Position: refs/heads/master@{#416238} |
