Summary: | [css-grid] Initial support for implicit grid before explicit grid | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Manuel Rego Casasnovas <rego> | ||||||||
Component: | Layout and Rendering | Assignee: | Manuel Rego Casasnovas <rego> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | commit-queue, jfernandez, simon.fraser, svillar | ||||||||
Priority: | P2 | ||||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 153488, 155199 | ||||||||||
Attachments: |
|
Description
Manuel Rego Casasnovas
2016-03-04 04:02:26 PST
Created attachment 273225 [details]
Patch
Comment on attachment 273225 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=273225&action=review > Source/WebCore/rendering/RenderGrid.cpp:1167 > + coordinate.rows.translate(abs(m_smallestRowStart)); Please use std::abs, which won’t silently convert a floating point value, or a uint32_t or size_t, to an int, rather than abs, which despite its generic name *will* do those things. For example, abs(4294967295U) returns 1, while std::abs(4294967295U) gives us a compile time error because of overload ambiguity. And abs(-1.1) returns 1, while std::abs(-1.1) returns 1.1. Also note that the return value of abs and std::abs can be negative if it’s passed the most negative integer. Not sure if this is an issue for the correctness of our code. > Source/WebCore/rendering/RenderGrid.cpp:1169 > + coordinate.columns.translate(abs(m_smallestColumnStart)); Ditto. > Source/WebCore/rendering/RenderGrid.cpp:1237 > + m_grid.grow(maximumRowIndex + abs(m_smallestRowStart)); Ditto. > Source/WebCore/rendering/RenderGrid.cpp:1239 > + column.grow(maximumColumnIndex + abs(m_smallestColumnStart)); Ditto. > Source/WebCore/rendering/style/GridCoordinate.h:168 > + if (resolvedInitialPosition >= 0) > + m_resolvedInitialPosition = std::min(resolvedInitialPosition, kGridMaxTracks - 1); > + else > + m_resolvedInitialPosition = std::max(resolvedInitialPosition, -kGridMaxTracks); I normally prefer to write this in a way that brackets the value with the minimum and maximum values. More elegant than comparing with zero, I think. m_resolvedInitialPosition = std::max(-kGridMaxTracks, std::min(resolvedInitialPosition, kGridMaxTracks - 1)); > Source/WebCore/rendering/style/GridCoordinate.h:172 > + if (resolvedFinalPosition >= 0) > + m_resolvedFinalPosition = std::min(resolvedFinalPosition, kGridMaxTracks); > + else > + m_resolvedFinalPosition = std::max(resolvedFinalPosition, -kGridMaxTracks + 1); Ditto. > Source/WebCore/rendering/style/GridResolvedPosition.cpp:253 > unsigned resolvedPosition = abs(position.integerPosition()) - 1; Same issue with abs rather than std::abs. Created attachment 273340 [details]
Patch
Thanks for the review. (In reply to comment #2) > Comment on attachment 273225 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=273225&action=review > > > Source/WebCore/rendering/RenderGrid.cpp:1167 > > + coordinate.rows.translate(abs(m_smallestRowStart)); > > Please use std::abs, which won’t silently convert a floating point value, or > a uint32_t or size_t, to an int, rather than abs, which despite its generic > name *will* do those things. > > For example, abs(4294967295U) returns 1, while std::abs(4294967295U) gives > us a compile time error because of overload ambiguity. And abs(-1.1) returns > 1, while std::abs(-1.1) returns 1.1. > > Also note that the return value of abs and std::abs can be negative if it’s > passed the most negative integer. Not sure if this is an issue for the > correctness of our code. Thanks for the explanation, I've changed it to std::abs() in all the different places. I think we're safe regarding the most negative integer as we're setting the limit in -kGridMaxTracks. > > Source/WebCore/rendering/style/GridCoordinate.h:168 > > + if (resolvedInitialPosition >= 0) > > + m_resolvedInitialPosition = std::min(resolvedInitialPosition, kGridMaxTracks - 1); > > + else > > + m_resolvedInitialPosition = std::max(resolvedInitialPosition, -kGridMaxTracks); > > I normally prefer to write this in a way that brackets the value with the > minimum and maximum values. More elegant than comparing with zero, I think. > > m_resolvedInitialPosition = std::max(-kGridMaxTracks, > std::min(resolvedInitialPosition, kGridMaxTracks - 1)); Done. Comment on attachment 273340 [details] Patch Rejecting attachment 273340 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'build', '--no-clean', '--no-update', '--build-style=release', '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: emMac.dia -c /Volumes/Data/EWS/WebKit/Source/WebCore/history/mac/HistoryItemMac.mm -o /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/HistoryItemMac.o ** BUILD FAILED ** The following build commands failed: CompileC /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/GridResolvedPosition.o rendering/style/GridResolvedPosition.cpp normal x86_64 c++ com.apple.compilers.llvm.clang.1_0.compiler (1 failure) Full output: http://webkit-queues.webkit.org/results/943725 Created attachment 273410 [details]
Patch
Comment on attachment 273410 [details] Patch Clearing flags on attachment: 273410 Committed r197850: <http://trac.webkit.org/changeset/197850> All reviewed patches have been landed. Closing bug. |