[webkit-reviews] review granted: [Bug 89881] Split map* functions out of StyleResolver into a helper object : [Attachment 149308] Fix Qt

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has granted Eric Seidel <eric at webkit.org>'s
request for review:
Bug 89881: Split map* functions out of StyleResolver into a helper object
https://bugs.webkit.org/show_bug.cgi?id=89881

Attachment 149308: Fix Qt
https://bugs.webkit.org/attachment.cgi?id=149308&action=review

------- Additional Comments from Daniel Bates <dbates at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=149308&action=review


> Source/WebCore/css/CSSToStyleMap.cpp:84
> +    case CSSValueFixed:
> +	   layer->setAttachment(FixedBackgroundAttachment);
> +	   break;
> +    case CSSValueScroll:
> +	   layer->setAttachment(ScrollBackgroundAttachment);
> +	   break;
> +    case CSSValueLocal:
> +	   layer->setAttachment(LocalBackgroundAttachment);
> +	   break;

I know that you're moving code. The compiler will probably already do this, we
should substitute "return" for "break" in each of these case statements since
we fall off the end of the function anyway after exiting the switch block.
Also, I tend to find that a switch block that is all returns or all breaks
tends to read better.

> Source/WebCore/css/CSSToStyleMap.cpp:266
> +    float zoomFactor = style()->effectiveZoom();
> +
> +    CSSPrimitiveValue* primitiveValue =
static_cast<CSSPrimitiveValue*>(value);
> +    Length length;
> +    if (primitiveValue->isLength())
> +	   length = primitiveValue->computeLength<Length>(style(),
rootElementStyle(), zoomFactor);
> +    else if (primitiveValue->isPercentage())
> +	   length = Length(primitiveValue->getDoubleValue(), Percent);
> +    else if (primitiveValue->isCalculatedPercentageWithLength())
> +	   length = Length(primitiveValue->cssCalcValue()->toCalcValue(style(),
rootElementStyle(), zoomFactor));
> +    else if (primitiveValue->isViewportPercentageLength())
> +	   length = primitiveValue->viewportPercentageLength();
> +    else
> +	   return;

This code is identical to code in CSSToStyleMap::mapFillXPosition. We consider
sharing such code in a follow up patch.

> Source/WebCore/css/CSSToStyleMap.cpp:572
> +    float zoom = useSVGZoomRules() ? 1.0f : style()->effectiveZoom();

Nit: It's sufficient to use 1 here instead of 1.0f.

> Source/WebCore/css/CSSToStyleMap.cpp:581
> +	   box.m_top = Length(slices->top()->getIntValue(), Relative);

LengthBox::m_{top, bottom, left, right} are all public instance variables. We
may want to consider renaming them {top, bottom, left, right} in a follow up
patch since LengthBox provides no encapsulation.

> Source/WebCore/css/CSSToStyleMap.h:86
> +}

Nit: We usually add an inline comment here of the form "// namespace WebCore"

> Source/WebCore/css/CSSToStyleMap.h:88
> +#endif

Nit: We usually add an inline comment here of the form "// CSSToStyleMap_h".