Bug 144442

Created attachment 252094 [details] Patch

Comment on attachment 252094 [details] Patch Attachment 252094 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/4876104951136256 New failing tests: fast/css/getComputedStyle/computed-style-without-renderer.html fast/css/getComputedStyle/computed-style.html svg/css/getComputedStyle-basic.xhtml

Created attachment 252103 [details] Archive of layout-test-results from ews105 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5

Comment on attachment 252094 [details] Patch Attachment 252094 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/4783139310272512 New failing tests: fast/css/getComputedStyle/computed-style-without-renderer.html fast/css/getComputedStyle/computed-style.html svg/css/getComputedStyle-basic.xhtml

Created attachment 252106 [details] Archive of layout-test-results from ews100 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-mavericks Platform: Mac OS X 10.9.5

Created attachment 252111 [details] Patch

Comment on attachment 252111 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=252111&action=review > Source/WebCore/WebCore.xcodeproj/project.pbxproj:22343 > + 9DAC7C521AF2CAA100437C44 /* CSSContentDistributionValue.cpp */, > + 9DAC7C531AF2CAA100437C44 /* CSSContentDistributionValue.h */, Please put this file in the list of files in alphabetical order according to the UNIX sort command. > Source/WebCore/WebCore.xcodeproj/project.pbxproj:23950 > + 9DAC7C571AF2CB6400437C44 /* StyleContentAlignmentData.h in Headers */, Ditto. > Source/WebCore/WebCore.xcodeproj/project.pbxproj:26102 > + 9DAC7C551AF2CAA200437C44 /* CSSContentDistributionValue.h in Headers */, Ditto. > Source/WebCore/WebCore.xcodeproj/project.pbxproj:28521 > + 9DAC7C541AF2CAA100437C44 /* CSSContentDistributionValue.cpp in Sources */, Ditto. > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1739 > + bool isFlex = element->style().isDisplayFlexibleBox(); Please inline the value of this variable into the line below. This variable is referenced exactly once in this function and I do not feel that its name adds any additional insight that is not already conveyed by its value, especially given the presence of the function name isDisplayFlexibleBox in he right hand side of this expression. > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1835 > +static PassRefPtr<CSSValueList> valueForContentPositionAndDistributionWithOverflowAlignment(ContentPosition position, OverflowAlignment overflowAlignment, ContentDistributionType distribution) The order of the parameters to this function disagrees with the order of the same parameters as implied by the name of this function. Specifically, the name of this function implies that the parameter overflowAlignment will be last. Either we should move parameter overflowAlignment to the end of the list of parameter to match the order implied by the function name or we should update the name of this function to reflect the order of the parameter list. > Source/WebCore/css/CSSContentDistributionValue.cpp:49 > + Please remove this empty line as I don't feel that it improves the readability of this code. > Source/WebCore/css/CSSContentDistributionValue.cpp:55 > + list->append(Ref<CSSValue>(*distribution())); > + if (m_position != CSSValueInvalid) > + list->append(Ref<CSSValue>(*position())); > + if (m_overflow != CSSValueInvalid) > + list->append(Ref<CSSValue>(*overflow())); See my remark in CSSContentDistributionValue.h with regards to making CSSContentDistributionValue::{distribution, position, overflow}() return a Ref instead of a PassRefPtr. Then we do not need to explicitly construct a Ref when calling CSSValueList::append(). > Source/WebCore/css/CSSContentDistributionValue.cpp:56 > + Ditto. > Source/WebCore/css/CSSContentDistributionValue.h:37 > + static PassRefPtr<CSSContentDistributionValue> create(CSSValueID distribution, CSSValueID position, CSSValueID overflow) Notice that PassRefPtr defines a move constructor that takes a Ref. We should have this return Ref<CSSContentDistributionValue> instead of PassRefPtr<CSSContentDistributionValue> since we want to remove PassRefPtr in the future per <https://lists.webkit.org/pipermail/webkit-dev/2014-December/027100.html> > Source/WebCore/css/CSSContentDistributionValue.h:45 > + PassRefPtr<CSSPrimitiveValue> distribution() const { return cssValuePool().createIdentifierValue(m_distribution); } > + PassRefPtr<CSSPrimitiveValue> position() const { return cssValuePool().createIdentifierValue(m_position); } > + PassRefPtr<CSSPrimitiveValue> overflow() const { return cssValuePool().createIdentifierValue(m_overflow); } Similarly, these functions should return a Ref instead of a PassRefPtr. > Source/WebCore/css/CSSParser.cpp:3235 > + return id == CSSValueSpaceBetween || id == CSSValueSpaceAround > + || id == CSSValueSpaceEvenly || id == CSSValueStretch; This is OK as-is. I would have written this return statement using one line instead of spanning it across two lines because there are not many disjuncts: return id == CSSValueSpaceBetween || id == CSSValueSpaceAround || id == CSSValueSpaceEvenly || id == CSSValueStretch; > Source/WebCore/css/CSSParser.cpp:3296 > + if (value->id == CSSValueAuto || isBaselinePositionKeyword(value->id)) { Can we assume value is always non-null? From briefly looking at other CSSParser::parse* functions it seems that m_valueList->current() can be null (e.g. CSSParser::parseLegacyPosition()). > Source/WebCore/css/CSSParser.cpp:3372 > + } else { > + return false; > } I know that you are preserving the style of this code. We omit braces for else clauses that have a one-line body. > Source/WebCore/rendering/style/RenderStyleConstants.h:255 > +enum ContentPosition {ContentPositionAuto, ContentPositionBaseline, ContentPositionLastBaseline, ContentPositionCenter, ContentPositionStart, ContentPositionEnd, ContentPositionFlexStart, ContentPositionFlexEnd, ContentPositionLeft, ContentPositionRight}; > +enum ContentDistributionType {ContentDistributionDefault, ContentDistributionSpaceBetween, ContentDistributionSpaceAround, ContentDistributionSpaceEvenly, ContentDistributionStretch}; Are we not able to make this enum classes? > Source/WebCore/rendering/style/StyleContentAlignmentData.h:30 > + Please remove this empty line. One empty line (above) is sufficient to demarcate the end of the header includes from the start of the C++ namespace.

Comment on attachment 252111 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=252111&action=review Thanks so much for the detailed review. >> Source/WebCore/WebCore.xcodeproj/project.pbxproj:22343 >> + 9DAC7C531AF2CAA100437C44 /* CSSContentDistributionValue.h */, > > Please put this file in the list of files in alphabetical order according to the UNIX sort command. Done. >> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1739 >> + bool isFlex = element->style().isDisplayFlexibleBox(); > > Please inline the value of this variable into the line below. > > This variable is referenced exactly once in this function and I do not feel that its name adds any additional insight that is not already conveyed by its value, especially given the presence of the function name isDisplayFlexibleBox in he right hand side of this expression. Done. >> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1835 >> +static PassRefPtr<CSSValueList> valueForContentPositionAndDistributionWithOverflowAlignment(ContentPosition position, OverflowAlignment overflowAlignment, ContentDistributionType distribution) > > The order of the parameters to this function disagrees with the order of the same parameters as implied by the name of this function. Specifically, the name of this function implies that the parameter overflowAlignment will be last. Either we should move parameter overflowAlignment to the end of the list of parameter to match the order implied by the function name or we should update the name of this function to reflect the order of the parameter list. Done. >> Source/WebCore/css/CSSContentDistributionValue.cpp:49 >> + > > Please remove this empty line as I don't feel that it improves the readability of this code. Done. >> Source/WebCore/css/CSSContentDistributionValue.cpp:55 >> + list->append(Ref<CSSValue>(*overflow())); > > See my remark in CSSContentDistributionValue.h with regards to making CSSContentDistributionValue::{distribution, position, overflow}() return a Ref instead of a PassRefPtr. Then we do not need to explicitly construct a Ref when calling CSSValueList::append(). Done. >> Source/WebCore/css/CSSContentDistributionValue.cpp:56 >> + > > Ditto. Done. >> Source/WebCore/css/CSSContentDistributionValue.h:37 >> + static PassRefPtr<CSSContentDistributionValue> create(CSSValueID distribution, CSSValueID position, CSSValueID overflow) > > Notice that PassRefPtr defines a move constructor that takes a Ref. We should have this return Ref<CSSContentDistributionValue> instead of PassRefPtr<CSSContentDistributionValue> since we want to remove PassRefPtr in the future per <https://lists.webkit.org/pipermail/webkit-dev/2014-December/027100.html> Done. >> Source/WebCore/css/CSSContentDistributionValue.h:45 >> + PassRefPtr<CSSPrimitiveValue> overflow() const { return cssValuePool().createIdentifierValue(m_overflow); } > > Similarly, these functions should return a Ref instead of a PassRefPtr. Done. >> Source/WebCore/css/CSSParser.cpp:3235 >> + || id == CSSValueSpaceEvenly || id == CSSValueStretch; > > This is OK as-is. I would have written this return statement using one line instead of spanning it across two lines because there are not many disjuncts: > > return id == CSSValueSpaceBetween || id == CSSValueSpaceAround || id == CSSValueSpaceEvenly || id == CSSValueStretch; Well., I can agree with that, but next functions with a similar logic span between several lines and this way all of them result in a similar length. >> Source/WebCore/css/CSSParser.cpp:3296 >> + if (value->id == CSSValueAuto || isBaselinePositionKeyword(value->id)) { > > Can we assume value is always non-null? From briefly looking at other CSSParser::parse* functions it seems that m_valueList->current() can be null (e.g. CSSParser::parseLegacyPosition()). Well, I think at some point it could be null, but now there is a check early returning if it is at the beginning of the CSSParser::parseValue method. I guess some parsing function still overprotect against null pointer access. I might be a good idea to do the same in this case, but I remember some previous reviews claiming it was not necessary to double check it. >> Source/WebCore/css/CSSParser.cpp:3372 >> } > > I know that you are preserving the style of this code. We omit braces for else clauses that have a one-line body. Done. >> Source/WebCore/rendering/style/RenderStyleConstants.h:255 >> +enum ContentDistributionType {ContentDistributionDefault, ContentDistributionSpaceBetween, ContentDistributionSpaceAround, ContentDistributionSpaceEvenly, ContentDistributionStretch}; > > Are we not able to make this enum classes? All of them are just enums. Besides, I'm not sure I totally understand what we could gain making them classes. >> Source/WebCore/rendering/style/StyleContentAlignmentData.h:30 >> + > > Please remove this empty line. One empty line (above) is sufficient to demarcate the end of the header includes from the start of the C++ namespace. Done.

Created attachment 252181 [details] Patch

Created attachment 252185 [details] Patch

Created attachment 252202 [details] Patch

Comment on attachment 252202 [details] Patch r=me

Comment on attachment 252202 [details] Patch Clearing flags on attachment: 252202 Committed r183748: <http://trac.webkit.org/changeset/183748>

All reviewed patches have been landed. Closing bug.