Bug 64464 – [CSS Exclusions] Fix for comment #23 on wrap-shape parsing bug 61726

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Alexandru Chiculita]

This is a fix for comment #23 from Tony Chang on bug 61726:

View in context: https://bugs.webkit.org/attachment.cgi?id=100567&action=review

> Source/WebCore/css/CSSParser.cpp:86
> +#include "CSSWrapShapes.h"

Nit: It's better to put the #if in the .h file and always include the file.  This reduces the number of #ifs and makes it easier for people to move files even if a feature is disabled.

> Source/WebCore/css/CSSParser.cpp:3742
> +        if (!valid)
> +            break;

Nit: Wouldn't it be simpler to just return 0 here?

> Source/WebCore/css/CSSParser.cpp:3765
> +                valid = false;
> +                break;

Nit: ... and here.

> Source/WebCore/css/CSSParser.cpp:3771
> +    if (!valid || argumentNumber < 3)

Nit: Then you wouldn't need the variable valid anymore.

> Source/WebCore/css/CSSWrapShapes.h:152
> +    PassRefPtr<CSSPrimitiveValue> getXAt(unsigned i) { return m_values.at(i << 1); }

Nit: Please use * 2 rather than bit shifting.

> Source/WebCore/css/CSSWrapShapes.h:153
> +    PassRefPtr<CSSPrimitiveValue> getYAt(unsigned i) { return m_values.at((i << 1) & 1); }

This looks wrong, isn't it always 0?  I would just write (i * 2) + 1.

[Alexandru Chiculita]

Created attachment 100685 [details]
Patch

[Tony Chang]

Comment on attachment 100685 [details]
Patch

thanks!

[WebKit Review Bot]

Comment on attachment 100685 [details]
Patch

Clearing flags on attachment: 100685

Committed r90937: <http://trac.webkit.org/changeset/90937>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.