Bug 156254

Created attachment 275990 [details] Patch

Comment on attachment 275990 [details] Patch Attachment 275990 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1120090 New failing tests: fast/events/right-click-focus.html svg/css/getComputedStyle-basic.xhtml editing/spelling/spellcheck-input-search-crash.html

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

Comment on attachment 275990 [details] Patch Attachment 275990 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/1120092 New failing tests: svg/css/getComputedStyle-basic.xhtml

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

Created attachment 275996 [details] Fixed layout tests failures.

Comment on attachment 275996 [details] Fixed layout tests failures. Attachment 275996 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1120396 New failing tests: fast/events/right-click-focus.html svg/css/getComputedStyle-basic.xhtml editing/spelling/spellcheck-input-search-crash.html

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

Comment on attachment 275996 [details] Fixed layout tests failures. Attachment 275996 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/1120403 New failing tests: svg/css/getComputedStyle-basic.xhtml

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

Created attachment 276003 [details] Fixed layout tests failures.

Comment on attachment 276003 [details] Fixed layout tests failures. Can someone help us figure out why the build is failing on Windows?

(In reply to comment #12) > Comment on attachment 276003 [details] > Fixed layout tests failures. > > Can someone help us figure out why the build is failing on Windows? The log doesn't provide any useful info to me. I had problems in a similar patch because of the all-in-one compiling (see comment 14 in bug #154282). Perhaps the same problem with this change ? diff --git a/Source/WebCore/rendering/RenderGrid.cpp b/Source/WebCore/rendering/RenderGrid.cpp index 66de23022f1dc569cf32b3170fd05068256ef6c2..b6faa65d8a992370edaa3b93f88a171317bd1dbd 100644 --- a/Source/WebCore/rendering/RenderGrid.cpp +++ b/Source/WebCore/rendering/RenderGrid.cpp @@ -38,6 +38,7 @@ namespace WebCore { static const int infinity = -1; +static const ItemPosition selfAlignmentNormalBehavior = ItemPositionStretch; diff --git a/Source/WebCore/rendering/RenderFlexibleBox.cpp b/Source/WebCore/rendering/RenderFlexibleBox.cpp index 69f3781a6b9327e59dff7e5f9f10d20c7c1a13d4..4ca043a369e7a431b8913622afc6ae7a85a85e0f 100644 --- a/Source/WebCore/rendering/RenderFlexibleBox.cpp +++ b/Source/WebCore/rendering/RenderFlexibleBox.cpp @@ -39,6 +39,8 @@ namespace WebCore { +static const ItemPosition selfAlignmentNormalBehavior = ItemPositionStretch; + I can send a patch with a different name for that variable.

Created attachment 279956 [details] Patch Trying to fix the win build

Created attachment 279957 [details] Patch Trying to fix the win build

Created attachment 279972 [details] Patch Trying to fix the win build

Comment on attachment 279972 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=279972&action=review > Source/WebCore/ChangeLog:32 > + (WebCore::CSSParser::parseItemPositionOverflowPosition): A new value 'normal' is now valid.a Stray "a" here. > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2281 > + return {data.position(), OverflowAlignmentDefault}; We’ve been using spaces in the braces in this syntax in WebKit coding style: return { data.position(), OverflowAlignmentDefault }; It’s not mandatory to match this, but would fit in better with other new code we are writing. > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2291 > + const StyleSelfAlignmentData& inheritedValue = (!parent || !parent->computedStyle()) ? RenderStyle::initialDefaultAlignment() : parent->computedStyle()->justifyItems(); Could use auto& or const auto& here instead to make this line not so long. Not sure it is important to state the type name explicitly. > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2296 > + return {ItemPositionNormal, OverflowAlignmentDefault}; Spaces in braces thing. > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2306 > + return {ItemPositionNormal, OverflowAlignmentDefault}; Spaces in braces thing. > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2317 > + return {ItemPositionNormal, OverflowAlignmentDefault}; Spaces in braces thing. > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2401 > +static RefPtr<CSSValueList> valueForItemPositionWithOverflowAlignment(const StyleSelfAlignmentData& data) This function should return a Ref, not. RefPtr. > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2404 > RefPtr<CSSValueList> result = CSSValueList::createSpaceSeparated(); This should use auto and be a Ref, not a RefPtr. > Source/WebCore/rendering/RenderFlexibleBox.cpp:42 > +static const ItemPosition selfAlignmentNormalBehaviorFlexibleBox = ItemPositionStretch; Maybe this can be constexpr instead of const. > Source/WebCore/rendering/RenderFlexibleBox.cpp:239 > + const auto& newStyle = style(); Consider just auto& instead of const auto&. > Source/WebCore/rendering/RenderFlexibleBox.cpp:241 > + const auto& childStyle = child->style(); Ditto. > Source/WebCore/rendering/RenderFlexibleBox.cpp:242 > + ItemPosition previousAlignment = childStyle.resolvedAlignSelf(*oldStyle, selfAlignmentNormalBehaviorFlexibleBox).position(); Consider auto. > Source/WebCore/rendering/RenderFlexibleBox.cpp:1116 > + ContentPosition position = style().resolvedJustifyContentPosition(contentAlignmentNormalBehaviorFlexibleBox()); > + ContentDistributionType distribution = style().resolvedJustifyContentDistribution(contentAlignmentNormalBehaviorFlexibleBox()); Consider auto for types. > Source/WebCore/rendering/RenderFlexibleBox.cpp:1200 > + ContentPosition position = style().resolvedJustifyContentPosition(contentAlignmentNormalBehaviorFlexibleBox()); > + ContentDistributionType distribution = style().resolvedJustifyContentDistribution(contentAlignmentNormalBehaviorFlexibleBox()); Consider auto for types. > Source/WebCore/rendering/RenderFlexibleBox.cpp:1258 > + ContentPosition position = style().resolvedAlignContentPosition(contentAlignmentNormalBehaviorFlexibleBox()); > + ContentDistributionType distribution = style().resolvedAlignContentDistribution(contentAlignmentNormalBehaviorFlexibleBox()); Consider auto for types. > Source/WebCore/rendering/RenderGrid.cpp:42 > static const int infinity = -1; > +static const ItemPosition selfAlignmentNormalBehavior = ItemPositionStretch; Consider constexpr instead of const. > Source/WebCore/rendering/RenderGrid.cpp:2047 > + OverflowAlignment overflow = child.style().resolvedAlignSelf(style(), selfAlignmentNormalBehavior).overflow(); > + LayoutUnit offsetFromStartPosition = computeOverflowAlignmentOffset(overflow, endOfRow - startOfRow, childBreadth); Consider auto for types. > Source/WebCore/rendering/RenderGrid.cpp:2080 > + OverflowAlignment overflow = child.style().resolvedJustifySelf(style(), selfAlignmentNormalBehavior).overflow(); > + LayoutUnit offsetFromStartPosition = computeOverflowAlignmentOffset(overflow, endOfColumn - startOfColumn, childBreadth); Consider auto for types. > Source/WebCore/rendering/RenderGrid.cpp:2143 > + ContentPosition position = isRowAxis ? style().resolvedJustifyContentPosition(contentAlignmentNormalBehaviorGrid()) : style().resolvedAlignContentPosition(contentAlignmentNormalBehaviorGrid()); > + ContentDistributionType distribution = isRowAxis ? style().resolvedJustifyContentDistribution(contentAlignmentNormalBehaviorGrid()) : style().resolvedAlignContentDistribution(contentAlignmentNormalBehaviorGrid()); Consider auto for types. > Source/WebCore/rendering/style/RenderStyle.cpp:197 > + return {normalValueBehavior, OverflowAlignmentDefault}; Spaces in braces. > Source/WebCore/rendering/style/RenderStyle.cpp:221 > + return {normalValueBehaviour, OverflowAlignmentDefault}; Spaces in braces. > Source/WebCore/rendering/style/RenderStyle.h:514 > + const StyleSelfAlignmentData resolvedAlignItems(ItemPosition normalValueBehaviour) const; > + const StyleSelfAlignmentData resolvedAlignSelf(const RenderStyle& parentStyle, ItemPosition normalValueBehaviour) const; > + const StyleSelfAlignmentData resolvedJustifyItems(ItemPosition normalValueBehaviour) const; > + const StyleSelfAlignmentData resolvedJustifySelf(const RenderStyle& parentStyle, ItemPosition normalValueBehaviour) const; Return types should just be StyleSelfAlignmentData, not const StyleSelfAlignmentData. It’s rarely useful to have a return value be a const; functions typically return "int", not "const int". > Source/WebCore/rendering/style/RenderStyleConstants.h:257 > +enum ItemPosition {ItemPositionAuto, ItemPositionNormal, ItemPositionStretch, ItemPositionBaseline, ItemPositionLastBaseline, ItemPositionCenter, ItemPositionStart, ItemPositionEnd, ItemPositionSelfStart, ItemPositionSelfEnd, ItemPositionFlexStart, ItemPositionFlexEnd, ItemPositionLeft, ItemPositionRight}; Spaces in braces here. Notice that the older code all has the spaces, but the newer code does not. Seems this code has been done without considering prevailing WebKit style.

Created attachment 280039 [details] Patch Patch for landing.

Created attachment 280041 [details] Patch Patch for landing fixing copy-elision issue.

(In reply to comment #18) > Created attachment 280039 [details] > Patch > > Patch for landing. This patch doesn't build in the ios-sim bot because the clang version used complains about using WTFMove in the return value of some functions : /Volumes/Data/EWS/WebKit/Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2411:12: error: moving a local object in a return statement prevents copy elision [-Werror,-Wpessimizing-move] return WTFMove(result); ^ In file included from /Volumes/Data/EWS/WebKit/Source/WebCore/css/CSSComputedStyleDeclaration.cpp:25: Apparently, this is issue has been already reported in llvm: https://llvm.org/bugs/show_bug.cgi?id=23819 There are similar returns in the same class, so I'm not sure what's the correct way to implement it. I've submitted a new patch without the WTFMove, but I wonder whether it will cause unnecessary ref/deref ops.

(In reply to comment #20) > I've submitted a new patch without the WTFMove, but I wonder whether it will > cause unnecessary ref/deref ops. It won’t. An explicit std::move is not needed or helpful when the class matches exactly and the return value optimization kicks in without requiring a move. I find remembering the rule annoying when using a compiler without the warning, but there’s no actual reference count churning.

Comment on attachment 280041 [details] Patch Clearing flags on attachment: 280041 Committed r201498: <http://trac.webkit.org/changeset/201498>

All reviewed patches have been landed. Closing bug.