[webkit-reviews] review denied: [Bug 42799] pixelWidth/posWidth are processed incorrectly : [Attachment 67151] patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Yuxiang Luo <luoyx at google.com>'s
request for review:
Bug 42799: pixelWidth/posWidth are processed incorrectly
https://bugs.webkit.org/show_bug.cgi?id=42799

Attachment 67151: patch
https://bugs.webkit.org/attachment.cgi?id=67151&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=67151&action=review

This code really ought to be factored so that the rules about pixel and pos are
in the DOM; the code inside the bindings is getting way too complex and is
doing things that should not be part of the bindings.

It’s OK to have the JavaScript bindings parse out the pixel and pos prefix, but
then the work of actually computing an appropriate value for both getting and
setting should be done in the DOM code, not the bindings. And we should
consider moving almost all of the code into the DOM class. The bindings can
just pass in the property name and an enum to indicate what the prefix is, and
the rest can be handled in the DOM classes. The rule of thumb is that the
bindings code should be about translating from the engine data structures to
the DOM, but most of the logic should be in the DOM class itself.

> WebCore/bindings/js/JSCSSStyleDeclarationCustom.cpp:85
> -static String cssPropertyName(const Identifier& propertyName, bool*
hadPixelOrPosPrefix = 0)
> +static String cssPropertyName(const Identifier& propertyName, bool*
hadPixelPrefix = 0, bool* hadPosPrefix = 0)

Since we support only one prefix, this should be a single enum, not a pair of
booleans.

> WebCore/bindings/js/JSCSSStyleDeclarationCustom.cpp:153
> +    // posTop returns "CSS top" as number value in original unit _if_ its a

Should be "it's" not "its".

> WebCore/bindings/js/JSCSSStyleDeclarationCustom.cpp:156
> +    bool pixel, pos;

We normally declare multiple variables like this on multiple lines.

> WebCore/bindings/js/JSCSSStyleDeclarationCustom.cpp:162
> +	       RefPtr<CSSPrimitiveValue> pv =
static_pointer_cast<CSSPrimitiveValue>(v);

This seems to do extra work that is not needed for the common case where we
have neither the pixel nor the pos prefix.

We normally prefer to use words rather than letters for local variables, and
this is something we try to stick with for new code.

I suggest primitiveValue rather than pv.

> WebCore/bindings/js/JSCSSStyleDeclarationCustom.cpp:164
> +		   // pixel* like pixelTop returns integer instead of float

The wording of this comment is confusing. One way to write this in plain
language is:

    // Properties with names with a pixel prefix, such as pixelTop, return
integer values, not floating point.

> WebCore/bindings/js/JSCSSStyleDeclarationCustom.cpp:166
> +		       RefPtr<Node> node = thisObj->impl()->getNode();

This should be a raw pointer, not a RefPtr.

> WebCore/bindings/js/JSCSSStyleDeclarationCustom.cpp:172
> +		       if (style && rootStyle)
> +			   return jsNumber(exec, pv->computeLengthInt(style,
rootStyle));
> +		       return jsNumber(exec, 0);

In WebKit we normally use a style called “early return”. In this coding style
you would write it like this:

    if (!style || !rootStyle)
	return jsNumber(exec, 0);
    return jsNumber(exec, primitiveValue->computeLengthInt(style, rootStyle));