https://drafts.csswg.org/css-logical/#border-radius-properties
<rdar://problem/70579854>
Created attachment 416234 [details] Patch
Comment on attachment 416234 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=416234&action=review > Source/WebCore/platform/text/WritingMode.h:193 > + if (isHorizontalWritingMode(writingMode) || isFlippedLinesWritingMode(writingMode)) Isn't this equivalent to !isFlippedBlockWritingMode() (aka: vertical-rl) ? > Source/WebCore/platform/text/WritingMode.h:210 > + if (isHorizontalWritingMode(writingMode) || isFlippedLinesWritingMode(writingMode)) Ditto > Source/WebCore/platform/text/WritingMode.h:221 > + if (isHorizontalWritingMode(writingMode) || isFlippedLinesWritingMode(writingMode)) Ditto > Source/WebCore/platform/text/WritingMode.h:226 > + if (isHorizontalWritingMode(writingMode) || isFlippedLinesWritingMode(writingMode)) Ditto
Comment on attachment 416234 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=416234&action=review You need to update some test expectations to include the new propoerties. > Source/WebCore/ChangeLog:7 > + Please add a longer description here, explaining which properties are being added as part of this patch.
Comment on attachment 416234 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=416234&action=review > Source/WebCore/platform/text/WritingMode.h:132 > +enum class LogicalRadiusCorner : uint8_t { In the future there might be more properties referencing corners, other than border radius. So it may be better to name the enums like LogicalBoxCorner and BoxCorner. Seems I missed this in the Chromium review...
Comment on attachment 416234 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=416234&action=review > Source/WebCore/css/CSSProperty.cpp:175 > + case CSSPropertyBorderStartStartRadius: { > + return resolveToPhysicalProperty(direction, writingMode, LogicalRadiusCorner::StartStart, borderRadiusShorthand()); > + } > + case CSSPropertyBorderStartEndRadius: { > + return resolveToPhysicalProperty(direction, writingMode, LogicalRadiusCorner::StartEnd, borderRadiusShorthand()); > + } > + case CSSPropertyBorderEndStartRadius: { > + return resolveToPhysicalProperty(direction, writingMode, LogicalRadiusCorner::EndStart, borderRadiusShorthand()); > + } > + case CSSPropertyBorderEndEndRadius: { > + return resolveToPhysicalProperty(direction, writingMode, LogicalRadiusCorner::EndEnd, borderRadiusShorthand()); > + } No need for braces on these. >> Source/WebCore/platform/text/WritingMode.h:132 >> +enum class LogicalRadiusCorner : uint8_t { > > In the future there might be more properties referencing corners, other than border radius. > So it may be better to name the enums like LogicalBoxCorner and BoxCorner. > Seems I missed this in the Chromium review... I concur. See BoxSide and LogicalBoxSide. > Source/WebCore/platform/text/WritingMode.h:187 > +inline RadiusCorner mapLogicalRadiusCornerToPhysicalRadiusCorner(TextFlow textflow, WritingMode writingMode, LogicalRadiusCorner logicalRadiusCorner) This can be called mapLogicalCornerToPhysicalCorner()
Created attachment 416325 [details] Patch
(In reply to Javier Fernandez from comment #3) > Comment on attachment 416234 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=416234&action=review > > > Source/WebCore/platform/text/WritingMode.h:193 > > + if (isHorizontalWritingMode(writingMode) || isFlippedLinesWritingMode(writingMode)) > > Isn't this equivalent to !isFlippedBlockWritingMode() (aka: vertical-rl) ? > > > Source/WebCore/platform/text/WritingMode.h:210 > > + if (isHorizontalWritingMode(writingMode) || isFlippedLinesWritingMode(writingMode)) > > Ditto > > > Source/WebCore/platform/text/WritingMode.h:221 > > + if (isHorizontalWritingMode(writingMode) || isFlippedLinesWritingMode(writingMode)) > > Ditto > > > Source/WebCore/platform/text/WritingMode.h:226 > > + if (isHorizontalWritingMode(writingMode) || isFlippedLinesWritingMode(writingMode)) > > Ditto Yes, it might be worthy introducing isFlippedBlockWritingMode() in WritingMode.h. As we discussed, I probably will leave this to a separate patch.
(In reply to Manuel Rego Casasnovas from comment #4) > Comment on attachment 416234 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=416234&action=review > > You need to update some test expectations to include the new propoerties. > > > Source/WebCore/ChangeLog:7 > > + > > Please add a longer description here, explaining which properties are being > added as part of this patch. Done
(In reply to Simon Fraser (smfr) from comment #6) > Comment on attachment 416234 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=416234&action=review > > > Source/WebCore/css/CSSProperty.cpp:175 > > + case CSSPropertyBorderStartStartRadius: { > > + return resolveToPhysicalProperty(direction, writingMode, LogicalRadiusCorner::StartStart, borderRadiusShorthand()); > > + } > > + case CSSPropertyBorderStartEndRadius: { > > + return resolveToPhysicalProperty(direction, writingMode, LogicalRadiusCorner::StartEnd, borderRadiusShorthand()); > > + } > > + case CSSPropertyBorderEndStartRadius: { > > + return resolveToPhysicalProperty(direction, writingMode, LogicalRadiusCorner::EndStart, borderRadiusShorthand()); > > + } > > + case CSSPropertyBorderEndEndRadius: { > > + return resolveToPhysicalProperty(direction, writingMode, LogicalRadiusCorner::EndEnd, borderRadiusShorthand()); > > + } > > No need for braces on these. > > >> Source/WebCore/platform/text/WritingMode.h:132 > >> +enum class LogicalRadiusCorner : uint8_t { > > > > In the future there might be more properties referencing corners, other than border radius. > > So it may be better to name the enums like LogicalBoxCorner and BoxCorner. > > Seems I missed this in the Chromium review... > > I concur. See BoxSide and LogicalBoxSide. > > > Source/WebCore/platform/text/WritingMode.h:187 > > +inline RadiusCorner mapLogicalRadiusCornerToPhysicalRadiusCorner(TextFlow textflow, WritingMode writingMode, LogicalRadiusCorner logicalRadiusCorner) > > This can be called mapLogicalCornerToPhysicalCorner() Code updated. Thank you!
(In reply to Oriol Brufau from comment #5) > Comment on attachment 416234 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=416234&action=review > > > Source/WebCore/platform/text/WritingMode.h:132 > > +enum class LogicalRadiusCorner : uint8_t { > > In the future there might be more properties referencing corners, other than > border radius. > So it may be better to name the enums like LogicalBoxCorner and BoxCorner. > Seems I missed this in the Chromium review... Updated. Thank you!
Created attachment 416327 [details] Patch
(In reply to zsun from comment #10) > (In reply to Simon Fraser (smfr) from comment #6) > > Comment on attachment 416234 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=416234&action=review > > > > > Source/WebCore/css/CSSProperty.cpp:175 > > > + case CSSPropertyBorderStartStartRadius: { > > > + return resolveToPhysicalProperty(direction, writingMode, LogicalRadiusCorner::StartStart, borderRadiusShorthand()); > > > + } > > > + case CSSPropertyBorderStartEndRadius: { > > > + return resolveToPhysicalProperty(direction, writingMode, LogicalRadiusCorner::StartEnd, borderRadiusShorthand()); > > > + } > > > + case CSSPropertyBorderEndStartRadius: { > > > + return resolveToPhysicalProperty(direction, writingMode, LogicalRadiusCorner::EndStart, borderRadiusShorthand()); > > > + } > > > + case CSSPropertyBorderEndEndRadius: { > > > + return resolveToPhysicalProperty(direction, writingMode, LogicalRadiusCorner::EndEnd, borderRadiusShorthand()); > > > + } > > > > No need for braces on these. > > > > >> Source/WebCore/platform/text/WritingMode.h:132 > > >> +enum class LogicalRadiusCorner : uint8_t { > > > > > > In the future there might be more properties referencing corners, other than border radius. > > > So it may be better to name the enums like LogicalBoxCorner and BoxCorner. > > > Seems I missed this in the Chromium review... I will try to update the Chromium changes. > > > > I concur. See BoxSide and LogicalBoxSide. > > > > > Source/WebCore/platform/text/WritingMode.h:187 > > > +inline RadiusCorner mapLogicalRadiusCornerToPhysicalRadiusCorner(TextFlow textflow, WritingMode writingMode, LogicalRadiusCorner logicalRadiusCorner) > > > > This can be called mapLogicalCornerToPhysicalCorner() > > Code updated. Thank you!
Created attachment 416330 [details] Patch
Created attachment 416335 [details] Patch
Comment on attachment 416335 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=416335&action=review > Source/WebCore/css/CSSProperties.json:1516 > + "border-corner-radius": { I don't see this property in https://www.w3.org/TR/css-logical/#border-radius-shorthands anywhere.
(In reply to Simon Fraser (smfr) from comment #16) > Comment on attachment 416335 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=416335&action=review > > > Source/WebCore/css/CSSProperties.json:1516 > > + "border-corner-radius": { > > I don't see this property in > https://www.w3.org/TR/css-logical/#border-radius-shorthands anywhere. You are right. It should be "border-radius" but "border-radius" is already used in this file to refer to physical radius corners. Any suggestion on naming this would be appreciated.
Comment on attachment 416335 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=416335&action=review >>> Source/WebCore/css/CSSProperties.json:1516 >>> + "border-corner-radius": { >> >> I don't see this property in https://www.w3.org/TR/css-logical/#border-radius-shorthands anywhere. > > You are right. It should be "border-radius" but "border-radius" is already used in this file to refer to physical radius corners. > > Any suggestion on naming this would be appreciated. Ziran, you seem to think that you need a shorthand for the logical border radius properties. Why? Isn't it possible to just remove this "border-corner-radius"? Your patch for Chromium didn't have it.
Created attachment 416404 [details] Patch
(In reply to Oriol Brufau from comment #18) > Comment on attachment 416335 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=416335&action=review > > >>> Source/WebCore/css/CSSProperties.json:1516 > >>> + "border-corner-radius": { > >> > >> I don't see this property in https://www.w3.org/TR/css-logical/#border-radius-shorthands anywhere. > > > > You are right. It should be "border-radius" but "border-radius" is already used in this file to refer to physical radius corners. > > > > Any suggestion on naming this would be appreciated. > > Ziran, you seem to think that you need a shorthand for the logical border > radius properties. Why? > Isn't it possible to just remove this "border-corner-radius"? > Your patch for Chromium didn't have it. Probably shouldn't be. Patch updated with it removed. Thank you!
Created attachment 416409 [details] Patch
(In reply to zsun from comment #8) > (In reply to Javier Fernandez from comment #3) > > Comment on attachment 416234 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=416234&action=review > > > > > Source/WebCore/platform/text/WritingMode.h:193 > > > + if (isHorizontalWritingMode(writingMode) || isFlippedLinesWritingMode(writingMode)) > > > > Isn't this equivalent to !isFlippedBlockWritingMode() (aka: vertical-rl) ? > > > > > Source/WebCore/platform/text/WritingMode.h:210 > > > + if (isHorizontalWritingMode(writingMode) || isFlippedLinesWritingMode(writingMode)) > > > > Ditto > > > > > Source/WebCore/platform/text/WritingMode.h:221 > > > + if (isHorizontalWritingMode(writingMode) || isFlippedLinesWritingMode(writingMode)) > > > > Ditto > > > > > Source/WebCore/platform/text/WritingMode.h:226 > > > + if (isHorizontalWritingMode(writingMode) || isFlippedLinesWritingMode(writingMode)) > > > > Ditto > > Yes, it might be worthy introducing isFlippedBlockWritingMode() in > WritingMode.h. As we discussed, I probably will leave this to a separate > patch. isFlippedWritingMode(WritingMode writingMode) has been define in WritingMode.h. I guess probably it's enough without introducing isFlippedBlockWritingMode(). Patch has been updated. Thank you!
Comment on attachment 416409 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=416409&action=review > Source/WebCore/platform/text/WritingMode.h:194 > + return BoxCorner::BottomLeft; There are some trailing spaces here > Source/WebCore/platform/text/WritingMode.h:206 > + return BoxCorner::TopRight; Ditto > Source/WebCore/platform/text/WritingMode.h:216 > + return BoxCorner::BottomRight; Ditto > Source/WebCore/platform/text/WritingMode.h:227 > + return BoxCorner::TopRight; Ditto
Created attachment 416432 [details] Patch
(In reply to Oriol Brufau from comment #23) > Comment on attachment 416409 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=416409&action=review > > > Source/WebCore/platform/text/WritingMode.h:194 > > + return BoxCorner::BottomLeft; > > There are some trailing spaces here > > > Source/WebCore/platform/text/WritingMode.h:206 > > + return BoxCorner::TopRight; > > Ditto > > > Source/WebCore/platform/text/WritingMode.h:216 > > + return BoxCorner::BottomRight; > > Ditto > > > Source/WebCore/platform/text/WritingMode.h:227 > > + return BoxCorner::TopRight; > > Ditto Updated. Thank you!
Comment on attachment 416432 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=416432&action=review > Source/WebCore/platform/text/WritingMode.h:181 > +inline BoxCorner mapLogicalCornerToPhysicalCorner(TextFlow textflow, WritingMode writingMode, LogicalBoxCorner logicalBoxCorner) Webkit has the non-standard '-webkit-writing-mode: horizontal-bt'. The mapping doesn't seem correct for that value. For example, with horizontal-bt ltr, the block-start side is the bottom, and the inline-start side is the left. But the start-start corner resolves to top-right instead of bottom-left.
Comment on attachment 416432 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=416432&action=review > Source/WebCore/platform/text/WritingMode.h:126 > +enum class LogicalBoxCorner : uint8_t { Maybe a comment here to describe the order (block/inline or inline/block?)
Comment on attachment 416432 [details] Patch r- based on Oriol's comment. Please be sure to add a test for -webkit-writing-mode: horizontal-bt
Created attachment 416517 [details] Patch
This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess
Created attachment 416519 [details] Patch
(In reply to Simon Fraser (smfr) from comment #27) > Comment on attachment 416432 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=416432&action=review > > > Source/WebCore/platform/text/WritingMode.h:126 > > +enum class LogicalBoxCorner : uint8_t { > > Maybe a comment here to describe the order (block/inline or inline/block?) Updated.
(In reply to Oriol Brufau from comment #26) > Comment on attachment 416432 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=416432&action=review > > > Source/WebCore/platform/text/WritingMode.h:181 > > +inline BoxCorner mapLogicalCornerToPhysicalCorner(TextFlow textflow, WritingMode writingMode, LogicalBoxCorner logicalBoxCorner) > > Webkit has the non-standard '-webkit-writing-mode: horizontal-bt'. The > mapping doesn't seem correct for that value. > For example, with horizontal-bt ltr, the block-start side is the bottom, and > the inline-start side is the left. > But the start-start corner resolves to top-right instead of bottom-left. Updated. It has modifies the imported WPT tests. I'm not sure if I should port it back.
The changes on tests related to horizontal-bt will be moved to a different patch. The following bug has been raised to address it: https://bugs.webkit.org/show_bug.cgi?id=220015
Comment on attachment 416519 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=416519&action=review > Source/WebCore/platform/text/WritingMode.h:183 > +inline BoxCorner mapLogicalCornerToPhysicalCorner(TextFlow textflow, WritingMode writingMode, LogicalBoxCorner logicalBoxCorner) I guess this function should be marked constexpr, like the others. And it seems strange to pass both TextFlow and WritingMode as arguments. I would pass the TextFlow only, though you may need a new isFlippedLinesTextFlow function. > Source/WebCore/platform/text/WritingMode.h:185 > + BoxCorner physicalCorner = static_cast<BoxCorner>(logicalBoxCorner); physicalCorner doesn't seem necessary. > Source/WebCore/platform/text/WritingMode.h:186 > + switch (logicalBoxCorner) { This switch seems very long. I think you can simplify the cases as such: switch (logicalBoxCorner) { case LogicalBoxCorner::StartStart: if (isFlippedTextFlow(textflow)) { if (isReversedTextFlow(textflow)) return BoxCorner::BottomRight; } else if (!isReversedTextFlow(textflow)) { return BoxCorner::TopLeft; } if (isFlippedLinesTextFlow(textflow)) return BoxCorner::BottomLeft; return BoxCorner::TopRight; case LogicalBoxCorner::StartEnd: if (isFlippedTextFlow(textflow)) { if (!isReversedTextFlow(textflow)) return BoxCorner::BottomRight; } else if (isReversedTextFlow(textflow)) { return BoxCorner::TopLeft; } if (isFlippedLinesTextFlow(textflow)) return BoxCorner::BottomLeft; return BoxCorner::TopRight; case LogicalBoxCorner::EndStart: if (isFlippedTextFlow(textflow)) { if (!isReversedTextFlow(textflow)) return BoxCorner::TopLeft; } else if (isReversedTextFlow(textflow)) { return BoxCorner::BottomRight; } if (isFlippedLinesTextFlow(textflow)) return BoxCorner::TopRight; return BoxCorner::BottomLeft; case LogicalBoxCorner::EndEnd: if (isFlippedTextFlow(textflow)) { if (isReversedTextFlow(textflow)) return BoxCorner::TopLeft; } else if (!isReversedTextFlow(textflow)) { return BoxCorner::BottomRight; } if (isFlippedLinesTextFlow(textflow)) return BoxCorner::TopRight; return BoxCorner::BottomLeft; } Or even remove the switch and use bool isBlockStart = logicalBoxCorner == LogicalBoxCorner::StartStart || logicalBoxCorner == LogicalBoxCorner::StartEnd; bool isInlineStart = logicalBoxCorner == LogicalBoxCorner::StartStart || logicalBoxCorner == LogicalBoxCorner::EndStart; if (isBlockStart == isFlippedTextFlow(textflow)) { if (isInlineStart == isReversedTextFlow(textflow)) return BoxCorner::BottomRight; } else if (isInlineStart != isReversedTextFlow(textflow)) { return BoxCorner::TopLeft; } if (isBlockStart == isFlippedLinesTextFlow(textflow)) return BoxCorner::BottomLeft; return BoxCorner::TopRight;
Created attachment 416669 [details] Patch
(In reply to Oriol Brufau from comment #35) > Comment on attachment 416519 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=416519&action=review > > > Source/WebCore/platform/text/WritingMode.h:183 > > +inline BoxCorner mapLogicalCornerToPhysicalCorner(TextFlow textflow, WritingMode writingMode, LogicalBoxCorner logicalBoxCorner) > > I guess this function should be marked constexpr, like the others. > > And it seems strange to pass both TextFlow and WritingMode as arguments. > I would pass the TextFlow only, though you may need a new > isFlippedLinesTextFlow function. > > > Source/WebCore/platform/text/WritingMode.h:185 > > + BoxCorner physicalCorner = static_cast<BoxCorner>(logicalBoxCorner); > > physicalCorner doesn't seem necessary. > > > Source/WebCore/platform/text/WritingMode.h:186 > > + switch (logicalBoxCorner) { > > This switch seems very long. I think you can simplify the cases as such: > > switch (logicalBoxCorner) { > case LogicalBoxCorner::StartStart: > if (isFlippedTextFlow(textflow)) { > if (isReversedTextFlow(textflow)) > return BoxCorner::BottomRight; > } else if (!isReversedTextFlow(textflow)) { > return BoxCorner::TopLeft; > } > if (isFlippedLinesTextFlow(textflow)) > return BoxCorner::BottomLeft; > return BoxCorner::TopRight; > case LogicalBoxCorner::StartEnd: > if (isFlippedTextFlow(textflow)) { > if (!isReversedTextFlow(textflow)) > return BoxCorner::BottomRight; > } else if (isReversedTextFlow(textflow)) { > return BoxCorner::TopLeft; > } > if (isFlippedLinesTextFlow(textflow)) > return BoxCorner::BottomLeft; > return BoxCorner::TopRight; > case LogicalBoxCorner::EndStart: > if (isFlippedTextFlow(textflow)) { > if (!isReversedTextFlow(textflow)) > return BoxCorner::TopLeft; > } else if (isReversedTextFlow(textflow)) { > return BoxCorner::BottomRight; > } > if (isFlippedLinesTextFlow(textflow)) > return BoxCorner::TopRight; > return BoxCorner::BottomLeft; > case LogicalBoxCorner::EndEnd: > if (isFlippedTextFlow(textflow)) { > if (isReversedTextFlow(textflow)) > return BoxCorner::TopLeft; > } else if (!isReversedTextFlow(textflow)) { > return BoxCorner::BottomRight; > } > if (isFlippedLinesTextFlow(textflow)) > return BoxCorner::TopRight; > return BoxCorner::BottomLeft; > } > > > Or even remove the switch and use > > bool isBlockStart = logicalBoxCorner == LogicalBoxCorner::StartStart || > logicalBoxCorner == LogicalBoxCorner::StartEnd; > bool isInlineStart = logicalBoxCorner == LogicalBoxCorner::StartStart || > logicalBoxCorner == LogicalBoxCorner::EndStart; > if (isBlockStart == isFlippedTextFlow(textflow)) { > if (isInlineStart == isReversedTextFlow(textflow)) > return BoxCorner::BottomRight; > } else if (isInlineStart != isReversedTextFlow(textflow)) { > return BoxCorner::TopLeft; > } > if (isBlockStart == isFlippedLinesTextFlow(textflow)) > return BoxCorner::BottomLeft; > return BoxCorner::TopRight; Code updated. Thank you!
Comment on attachment 416669 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=416669&action=review Thanks for working on this! > Source/WebCore/ChangeLog:37 > + (WebCore::mapLogicalCornerToPhysicalCorner): > 2020-12-22 Simon Fraser <simon.fraser@apple.com> Please leave a newline at the end here. > Source/WebCore/ChangeLog:70 > - (This saves a loop on the inline level boxes while constructing the lines at InlineContentBuilder::createDisplayLines) > + (This saves a loop on the inline level boxes while constructing the lines at InlineContentBuilder::createDisplayLines) Hey, this was probably just your editor going wild, but please leave the whitespace in the rest of the ChangeLog (or really anything you're not actively changing) alone. It isn't necessary to change.
Created attachment 417001 [details] Patch
(In reply to Sam Weinig from comment #38) > Comment on attachment 416669 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=416669&action=review > > Thanks for working on this! > > > Source/WebCore/ChangeLog:37 > > + (WebCore::mapLogicalCornerToPhysicalCorner): > > 2020-12-22 Simon Fraser <simon.fraser@apple.com> > > Please leave a newline at the end here. > > > Source/WebCore/ChangeLog:70 > > - (This saves a loop on the inline level boxes while constructing the lines at InlineContentBuilder::createDisplayLines) > > + (This saves a loop on the inline level boxes while constructing the lines at InlineContentBuilder::createDisplayLines) > > Hey, this was probably just your editor going wild, but please leave the > whitespace in the rest of the ChangeLog (or really anything you're not > actively changing) alone. It isn't necessary to change. Thanks very much for the comments! The files have been updated.
Created attachment 417005 [details] Patch
Comment on attachment 417005 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=417005&action=review > Source/WebCore/platform/text/WritingMode.h:90 > + return isFlippedTextFlow(textflow) != isVerticalTextFlow(textflow); You could make isFlippedLinesWritingMode call this function to avoid duplicating the logic: constexpr inline bool isFlippedLinesWritingMode(WritingMode writingMode) { return isFlippedLinesTextFlow(makeTextFlow(writingMode, TextDirection::LTR)); } > Source/WebCore/platform/text/WritingMode.h:190 > + BoxCorner physicalCorner = static_cast<BoxCorner>(logicalBoxCorner); physicalCorner doesn't seem needed. > Source/WebCore/platform/text/WritingMode.h:229 > + return physicalCorner; So I guess you are returning physicalCorner because some compiler was failing to notice that all cases are covered? Then I would add an ASSERT_NOT_REACHED() and just return some random corner like BoxCorner::TopLeft. I don't think static_cast<BoxCorner>(logicalBoxCorner) makes much sense. Or just replace the entire switch with bool isBlockStart = logicalBoxCorner == LogicalBoxCorner::StartStart || logicalBoxCorner == LogicalBoxCorner::StartEnd; bool isInlineStart = logicalBoxCorner == LogicalBoxCorner::StartStart || logicalBoxCorner == LogicalBoxCorner::EndStart; if (isBlockStart == isFlippedTextFlow(textflow)) { if (isInlineStart == isReversedTextFlow(textflow)) return BoxCorner::BottomRight; } else if (isInlineStart != isReversedTextFlow(textflow)) { return BoxCorner::TopLeft; } if (isBlockStart == isFlippedLinesTextFlow(textflow)) return BoxCorner::BottomLeft; return BoxCorner::TopRight;
Created attachment 417266 [details] Patch
(In reply to Oriol Brufau from comment #42) > Comment on attachment 417005 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=417005&action=review > > > Source/WebCore/platform/text/WritingMode.h:90 > > + return isFlippedTextFlow(textflow) != isVerticalTextFlow(textflow); > > You could make isFlippedLinesWritingMode call this function to avoid > duplicating the logic: > > constexpr inline bool isFlippedLinesWritingMode(WritingMode writingMode) > { > return isFlippedLinesTextFlow(makeTextFlow(writingMode, > TextDirection::LTR)); > } > > > Source/WebCore/platform/text/WritingMode.h:190 > > + BoxCorner physicalCorner = static_cast<BoxCorner>(logicalBoxCorner); > > physicalCorner doesn't seem needed. > > > Source/WebCore/platform/text/WritingMode.h:229 > > + return physicalCorner; > > So I guess you are returning physicalCorner because some compiler was > failing to notice that all cases are covered? > Then I would add an ASSERT_NOT_REACHED() and just return some random corner > like BoxCorner::TopLeft. > I don't think static_cast<BoxCorner>(logicalBoxCorner) makes much sense. > > Or just replace the entire switch with > > bool isBlockStart = logicalBoxCorner == LogicalBoxCorner::StartStart || > logicalBoxCorner == LogicalBoxCorner::StartEnd; > bool isInlineStart = logicalBoxCorner == LogicalBoxCorner::StartStart || > logicalBoxCorner == LogicalBoxCorner::EndStart; > if (isBlockStart == isFlippedTextFlow(textflow)) { > if (isInlineStart == isReversedTextFlow(textflow)) > return BoxCorner::BottomRight; > } else if (isInlineStart != isReversedTextFlow(textflow)) { > return BoxCorner::TopLeft; > } > if (isBlockStart == isFlippedLinesTextFlow(textflow)) > return BoxCorner::BottomLeft; > return BoxCorner::TopRight; Updated. Thank you!
Informal r+, but I'm not a reviewer.
Comment on attachment 417266 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=417266&action=review > Source/WebCore/css/CSSProperty.cpp:63 > +static CSSPropertyID resolveToPhysicalProperty(TextDirection direction, WritingMode writingMode, LogicalBoxCorner logicalBoxCorner, const StylePropertyShorthand& shorthand) > +{ > + return shorthand.properties()[static_cast<size_t>(mapLogicalCornerToPhysicalCorner(makeTextFlow(writingMode, direction), logicalBoxCorner))]; > +} This code is unsafe. If it's called with some random StylePropertyShorthand (not border-radius) then it can run off the end of the properties() array. Since you only call this for borderRadiusShorthand(), make it explicitly about border-radius.
Comment on attachment 417266 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=417266&action=review >> Source/WebCore/css/CSSProperty.cpp:63 >> +} > > This code is unsafe. If it's called with some random StylePropertyShorthand (not border-radius) then it can run off the end of the properties() array. > > Since you only call this for borderRadiusShorthand(), make it explicitly about border-radius. Good point, though the existing resolveToPhysicalProperty are also affected, aren't they? Maybe check the length() of the shorthand in a release assert for the functions that take a shorthand, and for the third function replace the `const CSSPropertyID* properties` parameter with `const CSSPropertyID (&properties)[2]`? That's basically what Chromium does.
Created attachment 417531 [details] Patch
Committed r271447: <https://trac.webkit.org/changeset/271447> All reviewed patches have been landed. Closing bug and clearing flags on attachment 417531 [details].