Bug 64464 - [CSS Exclusions] Fix for comment #23 on wrap-shape parsing bug 61726
Summary: [CSS Exclusions] Fix for comment #23 on wrap-shape parsing bug 61726
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-07-13 10:40 PDT by Alexandru Chiculita
Modified: 2011-07-13 11:56 PDT (History)
2 users (show)

See Also:


Attachments
Patch (8.90 KB, patch)
2011-07-13 10:52 PDT, Alexandru Chiculita
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexandru Chiculita 2011-07-13 10:40:03 PDT
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.
Comment 1 Alexandru Chiculita 2011-07-13 10:52:45 PDT
Created attachment 100685 [details]
Patch
Comment 2 Tony Chang 2011-07-13 10:56:40 PDT
Comment on attachment 100685 [details]
Patch

thanks!
Comment 3 WebKit Review Bot 2011-07-13 11:56:50 PDT
Comment on attachment 100685 [details]
Patch

Clearing flags on attachment: 100685

Committed r90937: <http://trac.webkit.org/changeset/90937>
Comment 4 WebKit Review Bot 2011-07-13 11:56:55 PDT
All reviewed patches have been landed.  Closing bug.