Bug 124618

Created attachment 217454 [details] Patch

Comment on attachment 217454 [details] Patch Attachment 217454 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/30578005

Comment on attachment 217454 [details] Patch Attachment 217454 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/30548002

The build failures are all because ASSERT_UNREACHED() doesn't do anything in a release build (And I only did a debug build locally). I've fixed it locally by initializing the variable in question, and will upload a new patch with that change (and any potential review feedback) in a bit.

Comment on attachment 217454 [details] Patch Attachment 217454 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/30508009

Comment on attachment 217454 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=217454&action=review ok though i don't really see how the new syntax is better > Source/WebCore/css/BasicShapeFunctions.cpp:85 > + return pool.createValue(center.length(), style); Not really in this patch but we need to get rid of CSSValues that take style. The factoring is just totally wrong. CSSValues are the input for constructing RenderStyle! > Source/WebCore/css/CSSBasicShapes.cpp:130 > + char opening[] = "circle("; > + char at[] = "at"; > + char separator[] = " "; > + StringBuilder result; > + // Compute the required capacity in advance to reduce allocations. > + result.reserveCapacity((sizeof(opening) - 1) + (3 * (sizeof(separator) - 1)) + 1 + radius.length() + sizeof(at) + centerX.length() + centerY.length()); There are probably some more modern ways to do this though I don't know what the recommended way is currently.

Comment on attachment 217454 [details] Patch Attachment 217454 [details] did not pass gtk-ews (gtk): Output: http://webkit-queues.appspot.com/results/30108016

Created attachment 217460 [details] Updated Patch

Comment on attachment 217460 [details] Updated Patch View in context: https://bugs.webkit.org/attachment.cgi?id=217460&action=review > Source/WebCore/css/CSSBasicShapes.cpp:177 > + m_centerY ? m_centerY->serializeResolvingVariables(variables) : String()) , (comma) instead of the last closing parenthesis.

Comment on attachment 217460 [details] Updated Patch Attachment 217460 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/29858025

Comment on attachment 217460 [details] Updated Patch Attachment 217460 [details] did not pass gtk-ews (gtk): Output: http://webkit-queues.appspot.com/results/30658011

(In reply to comment #9) > (From update of attachment 217460 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=217460&action=review > > > Source/WebCore/css/CSSBasicShapes.cpp:177 > > + m_centerY ? m_centerY->serializeResolvingVariables(variables) : String()) > > , (comma) instead of the last closing parenthesis. Thanks. I was about to ask why it compiled locally, then I realized that CSS_VARIABLES is turned off on the Mac port.

Created attachment 217462 [details] Patch Third time's the charm?

Comment on attachment 217462 [details] Patch Attachment 217462 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/29708030

Comment on attachment 217462 [details] Patch Attachment 217462 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/30048042

Created attachment 217471 [details] Patch Different warning settings on different ports make me a sad panda. Hopefully this one will pass all the EWS.

Comment on attachment 217471 [details] Patch Clearing flags on attachment: 217471 Committed r159585: <http://trac.webkit.org/changeset/159585>

All reviewed patches have been landed. Closing bug.

Comment on attachment 217471 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=217471&action=review > Source/WebCore/css/CSSBasicShapes.cpp:130 > + result.reserveCapacity((sizeof(opening) - 1) + (3 * (sizeof(separator) - 1)) + 1 + radius.length() + sizeof(at) + centerX.length() + centerY.length()); I’m not sure this optimization actually works. The sizeof(at) part seems wrong. > Source/WebCore/css/CSSBasicShapes.cpp:131 > + result.append(opening); This needs to use appendLiteral. > Source/WebCore/css/CSSBasicShapes.cpp:137 > + result.append(separator); This needs to use appendLiteral. > Source/WebCore/css/CSSBasicShapes.cpp:138 > + result.append(at); This needs to use appendLiteral. > Source/WebCore/css/CSSBasicShapes.cpp:139 > + result.append(separator); This needs to use appendLiteral. > Source/WebCore/css/CSSBasicShapes.cpp:141 > + result.append(separator); This needs to use appendLiteral. > Source/WebCore/css/CSSBasicShapes.cpp:144 > + result.append(")"); This needs to use appendLiteral. > Source/WebCore/css/CSSBasicShapes.cpp:145 > + if (box.length()) { This should be !box.isEmpty() > Source/WebCore/css/CSSBasicShapes.cpp:146 > + result.append(separator); This needs to use appendLiteral.

(In reply to comment #19) > (From update of attachment 217471 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=217471&action=review > > > Source/WebCore/css/CSSBasicShapes.cpp:130 > > + result.reserveCapacity((sizeof(opening) - 1) + (3 * (sizeof(separator) - 1)) + 1 + radius.length() + sizeof(at) + centerX.length() + centerY.length()); > > I’m not sure this optimization actually works. > > The sizeof(at) part seems wrong. > > > Source/WebCore/css/CSSBasicShapes.cpp:131 > > + result.append(opening); > > This needs to use appendLiteral. > > > Source/WebCore/css/CSSBasicShapes.cpp:137 > > + result.append(separator); > > This needs to use appendLiteral. > > > Source/WebCore/css/CSSBasicShapes.cpp:138 > > + result.append(at); > > This needs to use appendLiteral. > > > Source/WebCore/css/CSSBasicShapes.cpp:139 > > + result.append(separator); > > This needs to use appendLiteral. > > > Source/WebCore/css/CSSBasicShapes.cpp:141 > > + result.append(separator); > > This needs to use appendLiteral. > > > Source/WebCore/css/CSSBasicShapes.cpp:144 > > + result.append(")"); > > This needs to use appendLiteral. > > > Source/WebCore/css/CSSBasicShapes.cpp:145 > > + if (box.length()) { > > This should be !box.isEmpty() > > > Source/WebCore/css/CSSBasicShapes.cpp:146 > > + result.append(separator); > > This needs to use appendLiteral. I'm going to create a patch to implement the layout behavior. I'll make these changes in that patch.