Bug 166637

Created attachment 297887 [details] Patch

Created attachment 297888 [details] Patch

This has been ported from Blink: https://codereview.chromium.org/2546993002

Comment on attachment 297888 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=297888&action=review Looks like good work, but I am not saying review+ or review- because I have so many doubts. Link error saying this symbol can’t be found on the Apple platforms: "WebCore::CSSPrimitiveValue::doubleValue() const" The fix is to add WEBCORE_EXPORT to the doubleValue() function in CSSPrimitiveValue.h or change the code so it doesn’t call this. > Source/WebCore/ChangeLog:8 > + Avoid issues with very big values for the grid lines clamping them during parsing time. Is parsing time the right place for these kinds of limits? I’d like to hear what Hyatt has to say about that, unless you are absolutely sure. > Source/WebCore/css/parser/CSSPropertyParser.cpp:2988 > + if (numericValue) > + numericValue = CSSPrimitiveValue::create(clampTo(numericValue->intValue(), -kGridMaxTracks, kGridMaxTracks), CSSPrimitiveValue::UnitType::CSS_NUMBER); It seems unfortunate to create a new CSSPrimitiveValue object for this rather than clamping before creating the CSSPrimitiveValue object; feels unnecessarily inefficient. If clamping during parsing is going to be a normal pattern, can we make a consumeInteger function that does the clamping perhaps? > Tools/TestWebKitAPI/Tests/WebCore/CSSParser.cpp:48 > +static int getGridPositionInteger(CSSValueList& valueList) Normally we would not use "get" in a function name in WebKit unless the name came from the web platform. > Tools/TestWebKitAPI/Tests/WebCore/CSSParser.cpp:51 > + const auto* primitiveValue = downcast<CSSPrimitiveValue>(valueList.item(0)); Given this is a test the assertions aren’t great. We’d rather have the test system assertions instead of WebKit’s own internal assertions, I think. I would use auto* here; no need to explicitly write const. But also, why not use a reference instead a pointer? > Tools/TestWebKitAPI/Tests/WebCore/CSSParser.cpp:53 > + return primitiveValue->intValue(); Using intValue isn’t ideal for the test, because intValue itself does some clamping when it fetches the value. I suggest we use doubleValue() instead. Although perhaps the actual grid code uses intValue? (This seems a bit messy to me that we have are adding clamping in the parser and already have clamping in the intValue function.) Given that intValue itself clamps, I think the clamping might belong in the grid, not in the style parsing. Or in the style computation. It’s a bit peculiar to have clamping that is visible through the CSS object model. > Tools/TestWebKitAPI/Tests/WebCore/CSSParser.cpp:96 > +TEST(CSSPropertyParserTest, GridPositionLimits) Given that we implemented the clamping in the parser, this could be tested easily with code in JavaScript using the CSS object model. So it seems unnecessary to make kind of unit-test-style test for it. But I think maybe not in the parser. > Tools/TestWebKitAPI/Tests/WebCore/CSSParser.cpp:122 > + EXPECT_EQ(getGridPositionInteger(*downcast<CSSValueList>(value.get())), testCase.output); Normally we would write downcast<CSSValueList>(*value) instead of the thing written here.

Created attachment 298006 [details] Patch

Thanks for the review! (In reply to comment #4) > > Source/WebCore/ChangeLog:8 > > + Avoid issues with very big values for the grid lines clamping them during parsing time. > > Is parsing time the right place for these kinds of limits? I’d like to hear > what Hyatt has to say about that, unless you are absolutely sure. We are already clamping on parsing time for "repaet()" syntax. Hoever I've tried a different approach for this patch, clamping on GridPosition, which seems to be working fine too. I'm creating a new unit test to check the expected behavior. The reason to do a unit test instead of a regular layout test is to avoid creating a very huge grid to run the tests. That's why we originally created CSSParser.cpp test in bug #136217. PTAL to the new version.

BTW, when I reported the bug I forgot to mention how to reproduce it. It's enough with a very simple grid like: <!DOCTYPE html> <div style="display: grid;"> <div style="grid-column-start: 500000000000000000000000 foo;">test</div> </div>

(In reply to comment #6) > We are already clamping on parsing time for "repeat()" syntax. I believe this is a web-platform-observable choice. On the face of it, it seems always bad to me to change values as part of parsing them rather than in code that is interpreting the values. It reminds me of the kind of thing we did early in the WebKit project when we didn't understand the web platform as well as we do now. Thanks for the new patch, I think it’s better.

(In reply to comment #7) > BTW, when I reported the bug I forgot to mention how to reproduce it. > It's enough with a very simple grid like: > <!DOCTYPE html> > <div style="display: grid;"> > <div style="grid-column-start: 500000000000000000000000 foo;">test</div> > </div> Besides the unit test, I’d like to see a test like that one too.

Comment on attachment 298006 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=298006&action=review > Source/WebCore/rendering/style/GridPosition.h:136 > + void setIntegerPosition(int integerPosition) > + { > + m_integerPosition = clampTo(integerPosition, -kGridMaxTracks, kGridMaxTracks); > + } Normally we would put private function members before the data members, and we would put a blank line before "private:".

Created attachment 298042 [details] Patch for landing

Comment on attachment 298006 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=298006&action=review Uploaded patch for landing, adding layout test as requested. Thanks for the review. >> Source/WebCore/rendering/style/GridPosition.h:136 >> + } > > Normally we would put private function members before the data members, and we would put a blank line before "private:". Sure. Done.

Comment on attachment 298042 [details] Patch for landing Clearing flags on attachment: 298042 Committed r210320: <http://trac.webkit.org/changeset/210320>

All reviewed patches have been landed. Closing bug.

<rdar://problem/32571846>