Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14334014:
Parse "-webkit-columns: auto <length>" properly. (Closed)
|
Created:
7 years, 8 months ago by mstensho (USE GERRIT) Modified:
7 years, 4 months ago CC:
blink-reviews, apavlov+blink_chromium.org, darktears Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionParse "-webkit-columns: auto <length>" properly.
The two longhands for the 'columns' property ('column-count' and
'column-width') may both take 'auto' as a value. When we encounter
'auto' during parsing the value list of a declaration, we cannot just
make a guess at which property/properties that's meant for. Instead,
don't assign anything to 'auto' right away, but wait until all values
have been processed and at that point set the still unassigned
properties to 'auto'. If 'auto' isn't in the value list at all, set
unassigned properties to 'initial' for the 'columns' property, just
like we do for any other property.
R=
BUG=235415
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=155047
Patch Set 1 #
Total comments: 6
Patch Set 2 : Code review #Patch Set 3 : Rebase master + minor coding style fix that the old version of check-webkit-style didn't complain a… #
Total comments: 10
Patch Set 4 : Code review #2 #
Total comments: 6
Patch Set 5 : Code review #3 #
Total comments: 8
Patch Set 6 : Rebase master #Patch Set 7 : Address minor issues raised in the lgtm comment #Patch Set 8 : Compile fix #
Messages
Total messages: 18 (0 generated)
@darin: would you be the right person to review this, or should I choose someone else?
On 2013/07/19 21:01:00, Morten Stenshorne wrote: > @darin: would you be the right person to review this, or should I choose someone > else? No, I'm not the ideal reviewer for this change. +jchaffraix
https://codereview.chromium.org/14334014/diff/1/LayoutTests/fast/multicol/col... File LayoutTests/fast/multicol/columns-shorthand-parsing-2.html (right): https://codereview.chromium.org/14334014/diff/1/LayoutTests/fast/multicol/col... LayoutTests/fast/multicol/columns-shorthand-parsing-2.html:28: shouldBeEqualToString("document.getElementById('elm1').style.WebkitColumnWidth", "auto"); It would be more readable to move the repeated testing code to its own function. Maybe it would be better to do the setting in JavaScript using Element.style (not really sure about this one)? Nit: I usually prefer to test using window.getComputedStyle as Element.style returns nonsensical values sometimes (like I would have expected 'initial' and 'inherit' to be computed below but no). Not sure how well this would work though. https://codereview.chromium.org/14334014/diff/1/Source/core/css/CSSParser.cpp File Source/core/css/CSSParser.cpp (right): https://codereview.chromium.org/14334014/diff/1/Source/core/css/CSSParser.cpp... Source/core/css/CSSParser.cpp:3334: bool CSSParser::parseShorthand(CSSPropertyID propId, const StylePropertyShorthand& shorthand, bool important) I believe that's your problem: you are using the generic parseShorthand method and not a custom one that would keep track of what is set. Look at this file for some properties that use ShorthandScope and implement your own shorthand parsing. https://codereview.chromium.org/14334014/diff/1/Source/core/css/StyleProperty... File Source/core/css/StylePropertyShorthand.h (right): https://codereview.chromium.org/14334014/diff/1/Source/core/css/StyleProperty... Source/core/css/StylePropertyShorthand.h:39: StylePropertyShorthand(const CSSPropertyID* properties, unsigned numProperties, bool allAcceptAuto = false) NO! This class represents a generic shorthand, not a dumping ground for random extra field to handle special cases. Also, booleans are usually frowned upon as they make the code less readable.
https://codereview.chromium.org/14334014/diff/1/LayoutTests/fast/multicol/col... File LayoutTests/fast/multicol/columns-shorthand-parsing-2.html (right): https://codereview.chromium.org/14334014/diff/1/LayoutTests/fast/multicol/col... LayoutTests/fast/multicol/columns-shorthand-parsing-2.html:28: shouldBeEqualToString("document.getElementById('elm1').style.WebkitColumnWidth", "auto"); On 2013/07/23 00:56:29, Julien Chaffraix wrote: > It would be more readable to move the repeated testing code to its own function. > Maybe it would be better to do the setting in JavaScript using Element.style > (not really sure about this one)? > > Nit: I usually prefer to test using window.getComputedStyle as Element.style > returns nonsensical values sometimes (like I would have expected 'initial' and > 'inherit' to be computed below but no). Not sure how well this would work > though. Done. I think I want to keep using Element.style; that gives me the _specified_ property values (as long as they're part of the style attribute, of course). I don't want to look at the _computed_ values, since that's further from what the parser produces. https://codereview.chromium.org/14334014/diff/1/Source/core/css/CSSParser.cpp File Source/core/css/CSSParser.cpp (right): https://codereview.chromium.org/14334014/diff/1/Source/core/css/CSSParser.cpp... Source/core/css/CSSParser.cpp:3334: bool CSSParser::parseShorthand(CSSPropertyID propId, const StylePropertyShorthand& shorthand, bool important) On 2013/07/23 00:56:29, Julien Chaffraix wrote: > I believe that's your problem: you are using the generic parseShorthand method > and not a custom one that would keep track of what is set. > > Look at this file for some properties that use ShorthandScope and implement your > own shorthand parsing. Done. https://codereview.chromium.org/14334014/diff/1/Source/core/css/StyleProperty... File Source/core/css/StylePropertyShorthand.h (right): https://codereview.chromium.org/14334014/diff/1/Source/core/css/StyleProperty... Source/core/css/StylePropertyShorthand.h:39: StylePropertyShorthand(const CSSPropertyID* properties, unsigned numProperties, bool allAcceptAuto = false) On 2013/07/23 00:56:29, Julien Chaffraix wrote: > NO! > > This class represents a generic shorthand, not a dumping ground for random extra > field to handle special cases. > > Also, booleans are usually frowned upon as they make the code less readable. Done. Also, my patch doesn't even apply anymore, since StylePropertyShorthand.h is gone and is now auto-generated. :)
https://codereview.chromium.org/14334014/diff/19001/LayoutTests/fast/multicol... File LayoutTests/fast/multicol/columns-shorthand-parsing-2.html (right): https://codereview.chromium.org/14334014/diff/19001/LayoutTests/fast/multicol... LayoutTests/fast/multicol/columns-shorthand-parsing-2.html:13: ["-webkit-columns:auto 10em;", "auto", "10em"], Nit: You could remove the -webkit-columns in all these entries by changing your test function to: elm.style.cssText = "-webkit-column: " + test[0]; https://codereview.chromium.org/14334014/diff/19001/LayoutTests/fast/multicol... LayoutTests/fast/multicol/columns-shorthand-parsing-2.html:28: elm.style.cssText = test[0]; Let's avoid shortening the variable names, especially since it doesn't save much (apart from readability). https://codereview.chromium.org/14334014/diff/19001/Source/core/css/CSSParser... File Source/core/css/CSSParser-in.cpp (right): https://codereview.chromium.org/14334014/diff/19001/Source/core/css/CSSParser... Source/core/css/CSSParser-in.cpp:3219: bool propertyFound[]= { false, false }; This definitely works but this makes the code a lot more complicated than it ought to be. You expect only 2 values but the code uses a generic algorithm that could handle 40,000 of them for no good reason. How about we simplify the code by using 2 values here and avoiding unneeded loops? https://codereview.chromium.org/14334014/diff/19001/Source/core/css/CSSParser... Source/core/css/CSSParser-in.cpp:3239: propertyFound[propIndex] = found = true; This is a style violation, we use one statement per line. https://codereview.chromium.org/14334014/diff/19001/Source/core/css/CSSParser... Source/core/css/CSSParser-in.cpp:3268: addProperty(shorthand.properties()[i], value, important); The parseValue call above already calls addProperty so it's weird that we do it again here.
https://codereview.chromium.org/14334014/diff/19001/LayoutTests/fast/multicol... File LayoutTests/fast/multicol/columns-shorthand-parsing-2.html (right): https://codereview.chromium.org/14334014/diff/19001/LayoutTests/fast/multicol... LayoutTests/fast/multicol/columns-shorthand-parsing-2.html:13: ["-webkit-columns:auto 10em;", "auto", "10em"], On 2013/07/23 17:26:50, Julien Chaffraix wrote: > Nit: You could remove the -webkit-columns in all these entries by changing your > test function to: > > elm.style.cssText = "-webkit-column: " + test[0]; Yes, that would avoid some repetition, but wouldn't it just look weird in the cases where I actually have double declarations? "7 7em; -webkit-columns:auto auto auto;" https://codereview.chromium.org/14334014/diff/19001/LayoutTests/fast/multicol... LayoutTests/fast/multicol/columns-shorthand-parsing-2.html:28: elm.style.cssText = test[0]; On 2013/07/23 17:26:50, Julien Chaffraix wrote: > Let's avoid shortening the variable names, especially since it doesn't save much > (apart from readability). Done. https://codereview.chromium.org/14334014/diff/19001/Source/core/css/CSSParser... File Source/core/css/CSSParser-in.cpp (right): https://codereview.chromium.org/14334014/diff/19001/Source/core/css/CSSParser... Source/core/css/CSSParser-in.cpp:3219: bool propertyFound[]= { false, false }; On 2013/07/23 17:26:50, Julien Chaffraix wrote: > This definitely works but this makes the code a lot more complicated than it > ought to be. > > You expect only 2 values but the code uses a generic algorithm that could handle > 40,000 of them for no good reason. > > How about we simplify the code by using 2 values here and avoiding unneeded > loops? Done. https://codereview.chromium.org/14334014/diff/19001/Source/core/css/CSSParser... Source/core/css/CSSParser-in.cpp:3239: propertyFound[propIndex] = found = true; On 2013/07/23 17:26:50, Julien Chaffraix wrote: > This is a style violation, we use one statement per line. Done. https://codereview.chromium.org/14334014/diff/19001/Source/core/css/CSSParser... Source/core/css/CSSParser-in.cpp:3268: addProperty(shorthand.properties()[i], value, important); On 2013/07/23 17:26:50, Julien Chaffraix wrote: > The parseValue call above already calls addProperty so it's weird that we do it > again here. Here we set the unspecified or auto longhands that the parseValue() call didn't take care of. See parseShorthand(); it does something very similar. It's where I stole it from, after all. ;)
https://codereview.chromium.org/14334014/diff/26001/Source/core/css/CSSParser... File Source/core/css/CSSParser-in.cpp (right): https://codereview.chromium.org/14334014/diff/26001/Source/core/css/CSSParser... Source/core/css/CSSParser-in.cpp:3219: bool columnWidthFound = false; Ideally variable names should be good English: columnWidthWasFound or foundColumnWidth https://codereview.chromium.org/14334014/diff/26001/Source/core/css/CSSParser... Source/core/css/CSSParser-in.cpp:3247: Probably worth adding this assert: ASSERT(!columnCountFound || !columnWidthFound); https://codereview.chromium.org/14334014/diff/26001/Source/core/css/CSSParser... Source/core/css/CSSParser-in.cpp:3273: This code is way too complicated and I don't think the complexity is needed. You need to keep track of which longhand is set and what value those are. The only tricky case is if the first value is 'auto', the second one (or its absence) completely determines what's going to happen. If you thought of it in this way, it should be way more readable IMO. You could keep the 2 values as RefPtr<CSSValue> when you parse them (0 meaning implicit) and that would mean you just have to add them once (ie you need to extract the 'column-width' and 'column-count' parsing code into functions you can call). I also don't think the current handles 'auto' properly as you set 'column-count' to an implicit 'auto' not an implicit 'initial' value.
https://codereview.chromium.org/14334014/diff/26001/Source/core/css/CSSParser... File Source/core/css/CSSParser-in.cpp (right): https://codereview.chromium.org/14334014/diff/26001/Source/core/css/CSSParser... Source/core/css/CSSParser-in.cpp:3219: bool columnWidthFound = false; On 2013/07/24 21:41:11, Julien Chaffraix wrote: > Ideally variable names should be good English: columnWidthWasFound or > foundColumnWidth Done. https://codereview.chromium.org/14334014/diff/26001/Source/core/css/CSSParser... Source/core/css/CSSParser-in.cpp:3247: On 2013/07/24 21:41:11, Julien Chaffraix wrote: > Probably worth adding this assert: > > ASSERT(!columnCountFound || !columnWidthFound); Done. https://codereview.chromium.org/14334014/diff/26001/Source/core/css/CSSParser... Source/core/css/CSSParser-in.cpp:3273: On 2013/07/24 21:41:11, Julien Chaffraix wrote: > This code is way too complicated and I don't think the complexity is needed. > > You need to keep track of which longhand is set and what value those are. The > only tricky case is if the first value is 'auto', the second one (or its > absence) completely determines what's going to happen. If you thought of it in > this way, it should be way more readable IMO. > > You could keep the 2 values as RefPtr<CSSValue> when you parse them (0 meaning > implicit) and that would mean you just have to add them once (ie you need to > extract the 'column-width' and 'column-count' parsing code into functions you > can call). OK, I've tried to do this now. This eliminated the need for creating a ShorthandScope instance, so I removed it. Fine? > I also don't think the current handles 'auto' properly as you set 'column-count' > to an implicit 'auto' not an implicit 'initial' value. As long as the values are implicit, does it really matter that much whether they are 'auto' or 'initial', given that the initial value is indeed 'auto'? Anyway, there was some unnecessary complexity there, and I've tried to clean it up. I've removed setting of 'initial' completely now. That seems to be what the spec suggests, too. Also, this way we don't have to randomly choose which longhand should become 'initial' with 'columns:auto'. I also added checking of the -webkit-columns shorthand property to the tests. Seems to behave as desired. And I generally swapped the order from column-count-then-column-width to column-width-then-column-count, since this appears to be the canonical order.
lgtm https://codereview.chromium.org/14334014/diff/31001/LayoutTests/fast/multicol... File LayoutTests/fast/multicol/columns-shorthand-parsing-2.html (right): https://codereview.chromium.org/14334014/diff/31001/LayoutTests/fast/multicol... LayoutTests/fast/multicol/columns-shorthand-parsing-2.html:36: <body onload="runTests()"> Because you use the onload event to run the test, the output is out of order (the "PASS successfullyParsed is true" is before the test above). Several ways to solve that: - don't use the onload event and move the inline script just after #element so that it's accessible inline. - don't include js-test-post.js but set |successfullyParsed| to true and call finishJSTest() at the end of your test. The first solution is definitely better though :) https://codereview.chromium.org/14334014/diff/31001/Source/core/css/CSSParser... File Source/core/css/CSSParser-in.cpp (right): https://codereview.chromium.org/14334014/diff/31001/Source/core/css/CSSParser... Source/core/css/CSSParser-in.cpp:3249: if (columnWidth || !(columnWidth = parseColumnWidth())) { The second part of the 'if' is unusual in Blink. So I would prefer a slight refactoring on the way to do the parsing: if (!columnWidth) { if (columnWidth = parseColumnWidth()) continue; } else if (!columnCount) { if (columnCount = parseColumnCount()) continue; } else { // If we didn't find at least one match, this is an // invalid shorthand and we have to ignore it. return false; } It is a bit longer but it does underline the code flow a bit better IMHO. https://codereview.chromium.org/14334014/diff/31001/Source/core/css/CSSParser... Source/core/css/CSSParser-in.cpp:3261: // set. The one we don't set will get an implicit 'auto' value further down. Note that it does make a difference when using the web-inspector as one of the other will be considered implicit. That said, it's definitely not super critical. https://codereview.chromium.org/14334014/diff/31001/Source/core/css/CSSParser... Source/core/css/CSSParser-in.cpp:3275: ImplicitScope implicitScope(this, PropertyImplicit); Note that you don't necessarily need the ImplicitScope anymore. Just call: addProperty(CSSPropertyWebkitColumnWidth, cssValuePool().createIdentifierValue(CSSValueAuto), important, true /* implicit */)
OK, we're done. Thanks for the review! https://codereview.chromium.org/14334014/diff/31001/LayoutTests/fast/multicol... File LayoutTests/fast/multicol/columns-shorthand-parsing-2.html (right): https://codereview.chromium.org/14334014/diff/31001/LayoutTests/fast/multicol... LayoutTests/fast/multicol/columns-shorthand-parsing-2.html:36: <body onload="runTests()"> On 2013/07/26 20:14:39, Julien Chaffraix wrote: > Because you use the onload event to run the test, the output is out of order > (the "PASS successfullyParsed is true" is before the test above). Several ways > to solve that: > - don't use the onload event and move the inline script just after #element so > that it's accessible inline. > - don't include js-test-post.js but set |successfullyParsed| to true and call > finishJSTest() at the end of your test. > > The first solution is definitely better though :) Done. https://codereview.chromium.org/14334014/diff/31001/Source/core/css/CSSParser... File Source/core/css/CSSParser-in.cpp (right): https://codereview.chromium.org/14334014/diff/31001/Source/core/css/CSSParser... Source/core/css/CSSParser-in.cpp:3249: if (columnWidth || !(columnWidth = parseColumnWidth())) { On 2013/07/26 20:14:39, Julien Chaffraix wrote: > The second part of the 'if' is unusual in Blink. So I would prefer a slight > refactoring on the way to do the parsing: > > if (!columnWidth) { > if (columnWidth = parseColumnWidth()) > continue; > } else if (!columnCount) { > if (columnCount = parseColumnCount()) > continue; > } else { > // If we didn't find at least one match, this is an > // invalid shorthand and we have to ignore it. > return false; > } > > It is a bit longer but it does underline the code flow a bit better IMHO. Done. I omitted the "else"s, though. Otherwise it would fail miserably. :) https://codereview.chromium.org/14334014/diff/31001/Source/core/css/CSSParser... Source/core/css/CSSParser-in.cpp:3261: // set. The one we don't set will get an implicit 'auto' value further down. On 2013/07/26 20:14:39, Julien Chaffraix wrote: > Note that it does make a difference when using the web-inspector as one of the > other will be considered implicit. That said, it's definitely not super > critical. Right. That's undefined behavior. I guess it's problematic to have a shorthand with arbitrary order of the longhands when a certain value is legal for both longhands... I'll improve the comment, since it turns out that "doesn't really matter" is a bit on the assertive side. https://codereview.chromium.org/14334014/diff/31001/Source/core/css/CSSParser... Source/core/css/CSSParser-in.cpp:3275: ImplicitScope implicitScope(this, PropertyImplicit); On 2013/07/26 20:14:39, Julien Chaffraix wrote: > Note that you don't necessarily need the ImplicitScope anymore. Just call: > > addProperty(CSSPropertyWebkitColumnWidth, > cssValuePool().createIdentifierValue(CSSValueAuto), important, true /* implicit > */) Nice! I failed to notice that. Got totally blinded by the m_implicitShorthand thing, I suppose. Thanks. Parameters FTW! Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mstensho@opera.com/14334014/40001
Sorry for I got bad news for ya. Compile failed with a clobber build on mac_layout. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mstensho@opera.com/14334014/53001
Message was sent while issue was closed.
Change committed as 155047 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
