Bug 95930

Created attachment 162407 [details] Patch

Comment on attachment 162407 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162407&action=review I don't know enough about CSS variables to say anything about the correctness, but here are a few comments. The tests seem limited compared to the change. Isn't any of serializeResolvingVariables exposed to JavaScript through CSSOM? > Source/WebCore/css/CSSBasicShapes.cpp:64 > - if (m_radiusX.get()) { > + if (!radiusX.isEmpty()) { > result.appendLiteral(", "); > - result.append(m_radiusX->cssText()); > + result.append(radiusX); > > - if (m_radiusY.get()) { > + if (!radiusY.isEmpty()) { > result.appendLiteral(", "); > - result.append(m_radiusY->cssText()); > + result.append(radiusY); You should be using null string when there is no value. You could replace the String::isEmpty() by a the stronger constraint isNull(). > Source/WebCore/css/CSSBasicShapes.cpp:80 > + m_radiusX.get() ? m_radiusX->cssText() : emptyString(), > + m_radiusY.get() ? m_radiusY->cssText() : emptyString()); This should be using Sring() instead of emptyString(). > Source/WebCore/css/CSSBasicShapes.cpp:215 > +StringBuilder result; Indent. > Source/WebCore/css/CSSBasicShapes.cpp:216 > + result.reserveCapacity(32); This looks wrong. Please add a comment justifying the value. > Source/WebCore/css/Pair.h:53 > + static String pairString(const String& first, const String& second) The name is a bit misleading due to the first if(). > Source/WebCore/css/Pair.h:56 > + if (first == second) > + return first; This is so very strange. No idea if it is correct though :) > Source/WebCore/css/Pair.h:62 > + StringBuilder result; > + result.append(first); > + result.append(' '); > + result.append(second); > + return result.toString(); This should use String operators: return first + ' ' + second; Where there is no branch, the string operators often do a better job than SringBuilder. > Source/WebCore/css/Rect.h:89 > + StringBuilder result; > + result.append("rect("); > + result.append(top); > + result.append(' '); > + result.append(right); > + result.append(' '); > + result.append(bottom); > + result.append(' '); > + result.append(left); > + result.append(')'); > + return result.toString(); This should use String operators. > Source/WebCore/css/Rect.h:90 > + blank line...

Created attachment 162664 [details] Patch

You can't access variables through CSSOM yet. That behavior isn't actually specified by CSSWG yet, though I've been talking to Tab about it and we should have something on that front soon. All the code related feedback has been addressed in that last patch.

Comment on attachment 162664 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162664&action=review > Source/WebCore/ChangeLog:22 > + * css/CSSBasicShapes.cpp: > + (WebCore::buildRectangleString): > + (WebCore::CSSBasicShapeRectangle::cssText): > + (WebCore): > + (WebCore::CSSBasicShapeRectangle::serializeResolvingVariables): > + (WebCore::CSSBasicShapeRectangle::hasVariableReference): > + (WebCore::buildCircleString): > + (WebCore::CSSBasicShapeCircle::cssText): Please list a description of your change here. For example, you don't need to fill in all the hasVariableReference and serializeResolvingVariables sections, but in the first one you could mention the changes you're making. The changes in CSSPrimitiveValue.cpp and StyleResolver.cpp could use some explanation too. > Source/WebCore/css/CSSBasicShapes.cpp:151 > +String CSSBasicShapePolygon::cssText() const > +{ > StringBuilder result; The duplicated code with serializeResolvingVariables is unfortunate. Maybe you could make a helper function that takes a Vector<String> and cssTest/serializeResolvingVariables would pass in the constructed Vector? > Source/WebCore/css/CSSBasicShapes.cpp:160 > + for (unsigned i = 0; i < m_values.size(); i += 2) { Nit: unsigned -> size_t > Source/WebCore/css/CSSBasicShapes.cpp:185 > for (unsigned i = 0; i < m_values.size(); i += 2) { Nit: unsigned -> size_t > Source/WebCore/css/CSSBasicShapes.cpp:200 > + for (unsigned i = 0; i < m_values.size(); i++) { Nits: unsigned -> size_t, i++ -> ++i > Source/WebCore/css/Pair.h:53 > + static String generateCssString(const String& first, const String& second) Can this be private? Also, it should be generateCSSString. > Source/WebCore/css/Pair.h:55 > + if (first == second) Comparing strings seems like it would be slower than comparing CSSPrimitiveValues. I would probably just get rid of generateCSSString and duplicate the 'if values are the same don't append' logic since that's only 3 lines of code and will be different for serializeResolvingVariables. > Source/WebCore/css/Pair.h:69 > + return generateCssString(first()->customSerializeResolvingVariables(variables), > + second()->customSerializeResolvingVariables(variables)); Nit: Indention here is weird. > Source/WebCore/css/Rect.h:77 > + static String generateCssString(const String& top, const String& right, const String& bottom, const String& left) private, generateCSSString. > Source/WebCore/css/Rect.h:93 > + return generateCssString(top()->customSerializeResolvingVariables(variables), > + right()->customSerializeResolvingVariables(variables), > + bottom()->customSerializeResolvingVariables(variables), > + left()->customSerializeResolvingVariables(variables)); Nit: Indention is weird. > Source/WebCore/css/Rect.h:108 > + static String generateCssString(const String& top, const String& right, const String& bottom, const String& left) private, generateCSSString. > LayoutTests/ChangeLog:8 > + Add tests that use specialized CSS values (eg. pairs, quads). Can you add tests for all the different shapes (rect, basic shape rect, circle, ellipse, polygon)? > LayoutTests/fast/css/variables/root-background-size.html:4 > +try { internals.settings.setCSSVariablesEnabled(true); } catch (e) {} Why is this in a try/catch? Shouldn't we be checking for window.internals? > LayoutTests/fast/css/variables/var-inside-pair.html:4 > +internals.settings.setCSSVariablesEnabled(true); Please wrap this in a window.internals check. > LayoutTests/fast/css/variables/var-inside-quad.html:4 > +internals.settings.setCSSVariablesEnabled(true); Please wrap this in a window.internals check.

Created attachment 165498 [details] Patch

Comment on attachment 165498 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165498&action=review I didn’t review the whole patch, but I have a couple comment on some aspects of it. > Source/WebCore/css/CSSBasicShapes.cpp:40 > +static String buildRectangleString(const String& x, const String& y, const String& width, const String& height, const String& radiusX, const String& radiusY) This interface for the helper function guarantees lots of memory allocation and therefore slow performance. To call this we need to allocate a separate string buffer for each of these arguments and then free it as we append these all into a final string. We should not code this in a way that is so inefficient. Instead we need to change the way we build up CSS text so that instead of the cssText function we have an appendCSSText function that takes a StringBuilder object. The design pattern here is already bad, but the code change makes it even worse and increases the challenge of refactoring to fix it. > Source/WebCore/css/CSSBasicShapes.cpp:-43 > - result.reserveCapacity(32); Why remove this line of code? It’s a performance optimization, presumably. Did you measure and determine it’s not needed. > Source/WebCore/css/CSSBasicShapes.cpp:70 > + return buildRectangleString(m_x->cssText(), > + m_y->cssText(), > + m_width->cssText(), > + m_height->cssText(), > + m_radiusX.get() ? m_radiusX->cssText() : String(), > + m_radiusY.get() ? m_radiusY->cssText() : String()); Coding style guide specifically forbids this kind of function call formatting where we indent to match the function name. > Source/WebCore/css/CSSBasicShapes.cpp:179 > + points.append(m_values.at(i)->cssText()); This approach is intrinsically inefficient, allocating a separate memory buffer for the string form of each point and then destroying them all.

Comment on attachment 165498 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165498&action=review >> Source/WebCore/css/CSSBasicShapes.cpp:40 >> +static String buildRectangleString(const String& x, const String& y, const String& width, const String& height, const String& radiusX, const String& radiusY) > > This interface for the helper function guarantees lots of memory allocation and therefore slow performance. To call this we need to allocate a separate string buffer for each of these arguments and then free it as we append these all into a final string. > > We should not code this in a way that is so inefficient. Instead we need to change the way we build up CSS text so that instead of the cssText function we have an appendCSSText function that takes a StringBuilder object. The design pattern here is already bad, but the code change makes it even worse and increases the challenge of refactoring to fix it. Isn't this the same as before? Before we would call cssText() on each member variable which would also create a string buffer. I agree that it would be more efficient to switch to an appendCSSText style interface, but this applies to all the CSS Value classes, right? Maybe Luke would be willing to do that refactoring before landing this patch. >> Source/WebCore/css/CSSBasicShapes.cpp:179 >> + points.append(m_values.at(i)->cssText()); > > This approach is intrinsically inefficient, allocating a separate memory buffer for the string form of each point and then destroying them all. I agree that this is inefficient. The tradeoff is duplicating code inside the #if. It wasn't clear to me that this function was performance critical. It's only called if someone tries to read the CSS style values back out in JS or maybe if copying it to the pasteboard if it's inline.

Comment on attachment 165498 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165498&action=review >>> Source/WebCore/css/CSSBasicShapes.cpp:179 >>> + points.append(m_values.at(i)->cssText()); >> >> This approach is intrinsically inefficient, allocating a separate memory buffer for the string form of each point and then destroying them all. > > I agree that this is inefficient. The tradeoff is duplicating code inside the #if. It wasn't clear to me that this function was performance critical. It's only called if someone tries to read the CSS style values back out in JS or maybe if copying it to the pasteboard if it's inline. Actually, the previous code was also allocating a separate memory buffer for each string by calling cssText() on each value. The difference is in allocating the Vector.

Comment on attachment 165498 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165498&action=review >> Source/WebCore/css/CSSBasicShapes.cpp:70 >> + m_radiusY.get() ? m_radiusY->cssText() : String()); > > Coding style guide specifically forbids this kind of function call formatting where we indent to match the function name. I wasn't able to find this explicitly mentioned in the style guide, but I agree that this violates "The indent size is 4 spaces." I've written a patch for check-webkit-style to warn about this: https://bugs.webkit.org/show_bug.cgi?id=97602

Making cssText() write its output into a StringBuilder would make these complex cases faster, but perhaps it would make the simple (and probably more common) cases like simple values (eg. "3px") slower. We could support two versions of cssText() everywhere, but then you'd be duplicating code again which is also undesirable.

Comment on attachment 165498 [details] Patch Clearing flags on attachment: 165498 Committed r129579: <http://trac.webkit.org/changeset/129579>

All reviewed patches have been landed. Closing bug.

Reopening to attach new patch.

Created attachment 165730 [details] Patch

Comment on attachment 165730 [details] Patch OK, but normally we file a new bug for additional changes, even for small things like fixing indent style.

(In reply to comment #16) > (From update of attachment 165730 [details]) > OK, but normally we file a new bug for additional changes, even for small things like fixing indent style. This is confusing... I had cq+ then cleared the cq on the previous patch before it landed to make the changes, and then later in the day I webkit-patch uploaded a new patch... does webkit-patch upload insert the text "Reopening to attach new patch." on my behalf, 'cause I sure didn't do that myself.

(In reply to comment #17) > > This is confusing... I had cq+ then cleared the cq on the previous patch before it landed to make the changes, and then later in the day I webkit-patch uploaded a new patch... does webkit-patch upload insert the text "Reopening to attach new patch." on my behalf, 'cause I sure didn't do that myself. I think what happened is, we didn't clear the cq+ fast enough and the cq landed the patch and closed the bug. Later in the day, you uploaded a new patch which reopened the bug and added the comment (I think webkit-patch does this). I would close this bug and open a new bug with the style fixes.

(In reply to comment #8) > (From update of attachment 165498 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=165498&action=review > > >> Source/WebCore/css/CSSBasicShapes.cpp:40 > >> +static String buildRectangleString(const String& x, const String& y, const String& width, const String& height, const String& radiusX, const String& radiusY) > > > > This interface for the helper function guarantees lots of memory allocation and therefore slow performance. To call this we need to allocate a separate string buffer for each of these arguments and then free it as we append these all into a final string. > > > > We should not code this in a way that is so inefficient. Instead we need to change the way we build up CSS text so that instead of the cssText function we have an appendCSSText function that takes a StringBuilder object. The design pattern here is already bad, but the code change makes it even worse and increases the challenge of refactoring to fix it. > > Isn't this the same as before? Before we would call cssText() on each member variable which would also create a string buffer. What’s different is that this flawed strategy is now not only in the function, but also in the interface to the newly-added helper function.

(In reply to comment #10) > (From update of attachment 165498 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=165498&action=review > > >> Source/WebCore/css/CSSBasicShapes.cpp:70 > >> + m_radiusY.get() ? m_radiusY->cssText() : String()); > > > > Coding style guide specifically forbids this kind of function call formatting where we > indent to match the function name. > > I wasn't able to find this explicitly mentioned in the style guide, but I agree that this > violates "The indent size is 4 spaces." I've written a patch for check-webkit-style to > warn about this: https://bugs.webkit.org/show_bug.cgi?id=97602 As Luke mentioned in bug 97602 (see [1]), I also think that "the style guide seems to refer more to new blocks of code than to aligning successive lines in a case like a function call." This issue is usually causing some confusion (see [1], for instance) since bug 97602 got fixed, and so I'd be very interested in clarifying what we could do from this point on. Should we fix the check-webkit-style script? Should we force all newly written code to be written this way? Should we add an exception for some cases (e.g. GTK+ API)? Opinions are very welcome. I understand Darin's experience in WebKit is huge and so he's probably right on his statement, but I have to ask anyway to dissipate any doubt on this regard. Thanks in advance. [1] https://bugs.webkit.org/show_bug.cgi?id=97602#c9 [2] https://bugs.webkit.org/show_bug.cgi?id=98874#c14)

We should make an exception for the GTK+ API. Please file a bug against check-webkit-style and CC me.

(In reply to comment #21) > We should make an exception for the GTK+ API. Please file a bug against > check-webkit-style and CC me. Thanks for replying. I understand then that this is the expected indentation for a multiline statement from now on in general: fooBarFunction(foobarbaz, foobarbaz); Darin, as the one who first raised this issue, could you comment as well on this?