[Webkit-unassigned] [Bug 42799] pixelWidth/posWidth are processed incorrectly

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=42799





--- Comment #13 from Yuxiang Luo <luoyx at google.com>  2010-09-23 07:32:58 PST ---
(In reply to comment #11)
> (From update of attachment 67151 [details])
> 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.

Hi,darin

Thanks very much for the review. Do you mean the handling of pixel/pos should be put in CSSStyleDeclaration class? However, I don't think it should be put there, because pixel/pos-prefix properties are not CSS property, but CSSOM property accessed by script. And, if we put the logic in there, when setting 'pixelWidth', the CSSStyleDeclaration::setProperty will be invoked as following:
  setProperty('width', PREFIX_PIXEL, 100, ...);
The interface then looks very weird, people don't know what the interface intends to do.

Isn't it better to let CSSStyleDeclaration do only what it should do, that is to get/set primitive css properties? It might be better to put transformation code between primitive css properties and cssom properties in 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));

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.