Bug 157260

Created attachment 277900 [details] Patch

Comment on attachment 277900 [details] Patch Attachment 277900 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1255028 New failing tests: fast/css-grid-layout/grid-item-end-after-get-set.html fast/css-grid-layout/named-grid-lines-with-named-grid-areas-resolution.html fast/css-grid-layout/grid-container-change-named-grid-lines-recompute-child.html fast/css-grid-layout/grid-item-named-grid-line-resolution.html fast/css-grid-layout/grid-item-unknown-named-grid-line-resolution.html fast/css-grid-layout/grid-item-area-get-set.html fast/css-grid-layout/grid-item-start-before-get-set.html fast/css-grid-layout/grid-item-bad-named-area-auto-placement.html fast/css-grid-layout/grid-item-named-grid-area-resolution.html fast/css-grid-layout/named-grid-lines-with-named-grid-areas-get-set.html

Created attachment 277904 [details] Archive of layout-test-results from ews116 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-yosemite Platform: Mac OS X 10.10.5

Comment on attachment 277900 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=277900&action=review Multiple tests crashing. Presumably hitting assertions since the failures are only on the debug build. Please do not land until that is resolved. > Source/WebCore/rendering/RenderGrid.cpp:1190 > + // TODO(svillar): implement the algorithm to compute the number of auto repeat tracks. Even for comments like this, we normally use FIXME, not TODO, and we don’t put our names in these kinds of comments. We also use sentence capitalization. > Source/WebCore/rendering/RenderGrid.cpp:1191 > + UNUSED_PARAM(direction); Even in the short term line this, we normally like to omit or comment out the argument names rather than using UNUSED_PARAM. > Source/WebCore/rendering/RenderGrid.h:69 > + size_t autoRepeatCountForDirection(GridTrackSizingDirection direction) const > + { > + return direction == ForColumns ? m_autoRepeatColumns : m_autoRepeatRows; > + } When function reaches there point where it’s multiple lines like this, we almost always put the inline function outside and after the class definition with the inline keyword so the class definition itself is easier to read. > Source/WebCore/rendering/style/GridPositionsResolver.cpp:88 > + if (!gridLineNames.isEmpty()) { > + auto it = gridLineNames.find(namedLine); > + m_namedLinesIndexes = it == gridLineNames.end() ? nullptr : &it->value; > + } > + > + if (!autoRepeatGridLineNames.isEmpty()) { > + auto it = autoRepeatGridLineNames.find(namedLine); > + m_autoRepeatNamedLinesIndexes = it == autoRepeatGridLineNames.end() ? nullptr : &it->value; > + } Why the isEmpty checks? The find function should be fast enough without a special case for empty. I think this would probably be cleaner if it was done with helper functions and member initialization rather than with a function body. , m_namedLinesIndexes(findNamedLinesIndices(direction, gridContainerStyle)) , m_autoRepeatNamedLinesIndexes(findAutoRepeatNamedLinesIndices(direction, gridContainerStyle)) , m_insertionPoint(insertionPoint(direction, gridContainerStyle)) > Source/WebCore/rendering/style/GridPositionsResolver.cpp:93 > +bool NamedLineCollection::isValidNamedLineOrArea(const String& namedLine, const RenderStyle& gridContainerStyle, GridPositionSide side) Should be const. > Source/WebCore/rendering/style/GridPositionsResolver.cpp:100 > + if (gridLineNames.contains(namedLine) || autoRepeatGridLineNames.contains(namedLine)) > + return true; Do we have a guarantee that namedLine is not a null string? > Source/WebCore/rendering/style/GridPositionsResolver.cpp:103 > + String implicitName = implicitNamedGridLineForSide(namedLine, side); > + return gridLineNames.contains(implicitName) || autoRepeatGridLineNames.contains(implicitName); Do we have a guarantee that implicitName is not a null string? > Source/WebCore/rendering/style/GridPositionsResolver.cpp:106 > +bool NamedLineCollection::hasNamedLines() Should be const. > Source/WebCore/rendering/style/GridPositionsResolver.cpp:111 > +size_t NamedLineCollection::find(unsigned line) Should be const. > Source/WebCore/rendering/style/GridPositionsResolver.cpp:127 > + return m_autoRepeatNamedLinesIndexes->find(static_cast<unsigned>(1)); 1u is probably better than static_cast<unsigned>(1). Not sure why just 1 doesn’t work. > Source/WebCore/rendering/style/GridPositionsResolver.cpp:128 > + size_t position = m_autoRepeatNamedLinesIndexes->find(static_cast<unsigned>(0)); 0u is probably better than static_cast<unsigned>(0). Not sure why just 1 doesn’t work. > Source/WebCore/rendering/style/GridPositionsResolver.cpp:131 > + return localIndex ? m_autoRepeatNamedLinesIndexes->find(static_cast<unsigned>(1)) : notFound; 1u is probably better than static_cast<unsigned>(1). Not sure why just 1 doesn’t work. > Source/WebCore/rendering/style/GridPositionsResolver.cpp:137 > +bool NamedLineCollection::contains(unsigned line) Should be const. > Source/WebCore/rendering/style/GridPositionsResolver.cpp:139 > + RELEASE_ASSERT(hasNamedLines()); Why does this need to be a RELEASE_ASSERT? Normally we do not use those. > Source/WebCore/rendering/style/GridPositionsResolver.cpp:145 > + RELEASE_ASSERT(hasNamedLines()); Why does this need to be a RELEASE_ASSERT? Normally we do not use those. > Source/WebCore/rendering/style/GridPositionsResolver.h:59 > + bool hasNamedLines(); > + unsigned firstPosition(); > + > + bool contains(unsigned line); These members should all be const. > Source/WebCore/rendering/style/GridPositionsResolver.h:60 > +private: Should have a blank line before this "private". > Source/WebCore/rendering/style/GridPositionsResolver.h:63 > + size_t find(unsigned line); > + const Vector<unsigned>* m_namedLinesIndexes = nullptr; > + const Vector<unsigned>* m_autoRepeatNamedLinesIndexes = nullptr; I suggest a blank line between the function and the data members. We’ve been using the { nullptr } syntax rather than the = nullptr syntax. Not sure why.

(In reply to comment #4) > Comment on attachment 277900 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=277900&action=review > > Multiple tests crashing. Presumably hitting assertions since the failures > are only on the debug build. Please do not land until that is resolved. Sure, I'll take a look > > Source/WebCore/rendering/RenderGrid.cpp:1190 > > + // TODO(svillar): implement the algorithm to compute the number of auto repeat tracks. > > Even for comments like this, we normally use FIXME, not TODO, and we don’t > put our names in these kinds of comments. We also use sentence > capitalization. Yeah sorry about that, living in between chromium and webkit makes me commit this kind of mistakes from time to time. > > Source/WebCore/rendering/style/GridPositionsResolver.cpp:100 > > + if (gridLineNames.contains(namedLine) || autoRepeatGridLineNames.contains(namedLine)) > > + return true; > > Do we have a guarantee that namedLine is not a null string? namedLine is a reference so it can't be null. > > Source/WebCore/rendering/style/GridPositionsResolver.cpp:103 > > + String implicitName = implicitNamedGridLineForSide(namedLine, side); > > + return gridLineNames.contains(implicitName) || autoRepeatGridLineNames.contains(implicitName); > > Do we have a guarantee that implicitName is not a null string? Yes, the function used to construct it always returns a non-null non-empty string > > Source/WebCore/rendering/style/GridPositionsResolver.cpp:127 > > + return m_autoRepeatNamedLinesIndexes->find(static_cast<unsigned>(1)); > > 1u is probably better than static_cast<unsigned>(1). Not sure why just 1 > doesn’t work. > > > Source/WebCore/rendering/style/GridPositionsResolver.cpp:128 > > + size_t position = m_autoRepeatNamedLinesIndexes->find(static_cast<unsigned>(0)); > > 0u is probably better than static_cast<unsigned>(0). Not sure why just 1 > doesn’t work. > > > Source/WebCore/rendering/style/GridPositionsResolver.cpp:131 > > + return localIndex ? m_autoRepeatNamedLinesIndexes->find(static_cast<unsigned>(1)) : notFound; > > 1u is probably better than static_cast<unsigned>(1). Not sure why just 1 > doesn’t work. At least clang is very picky with this kind of unsigned vs signed comparisons. I'll use the 'u' suffix as the representation is indeed more compact and clean. > > Source/WebCore/rendering/style/GridPositionsResolver.cpp:137 > > +bool NamedLineCollection::contains(unsigned line) > > Should be const. > > > Source/WebCore/rendering/style/GridPositionsResolver.cpp:139 > > + RELEASE_ASSERT(hasNamedLines()); > > Why does this need to be a RELEASE_ASSERT? Normally we do not use those. > > > Source/WebCore/rendering/style/GridPositionsResolver.cpp:145 > > + RELEASE_ASSERT(hasNamedLines()); > > Why does this need to be a RELEASE_ASSERT? Normally we do not use those. No special reason, I'll change both to ASSERT.

(In reply to comment #4) > Comment on attachment 277900 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=277900&action=review > > Multiple tests crashing. Presumably hitting assertions since the failures > are only on the debug build. Please do not land until that is resolved. It was a stupid mistake cause by a rename. We moved from a method called isNonExistentNamedLineXXX() to isValidNamedLineXXX() There is an ASSERT() using that method where I renamed it properly but forgot to prepend the ! before the method call (because it's now returning false when it used to return true and viceversa).

Created attachment 277922 [details] Patch Patch for landing

Committed r200368: <http://trac.webkit.org/changeset/200368>