[webkit-reviews] review denied: [Bug 73000] Implement CSSPropertySize in CSSStyleApplyProperty. : [Attachment 116311] Patch

[bugzilla-daemon at webkit.org]

Andreas Kling <kling at webkit.org> has denied Luke Macpherson
<macpherson at chromium.org>'s request for review:
Bug 73000: Implement CSSPropertySize in CSSStyleApplyProperty.
https://bugs.webkit.org/show_bug.cgi?id=73000

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

------- Additional Comments from Andreas Kling <kling at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=116311&action=review


> Source/WebCore/ChangeLog:8
> +	   This refactoring moves the implementation of the page size
calculation into CSSStyleApplyPropwerty

Typo: CSSStyleApplyProperty.

> Source/WebCore/ChangeLog:11
> +	   No new tests / refactoing only.

Typo: refactoring.

> Source/WebCore/css/CSSStyleApplyProperty.cpp:680
> +    static bool pageSizeFromName(CSSPrimitiveValue* pageSizeName,
CSSPrimitiveValue* pageOrientation, Length& width, Length& height)

I believe this function should be called getPageSizeFromName() since it doesn't
return a "page size" directly, but rather fills in out arguments.

> Source/WebCore/css/CSSStyleApplyProperty.cpp:692
> +	   if (!pageSizeName || pageSizeName->primitiveType() !=
CSSPrimitiveValue::CSS_IDENT)
> +	       return false;

The primitiveType() check here is redundant, since getIdent() would return 0
for non-CSS_IDENT values below and bail via the default case.

> Source/WebCore/css/CSSStyleApplyProperty.cpp:733
> +	       if (pageOrientation->primitiveType() !=
CSSPrimitiveValue::CSS_IDENT)
> +		   return false;

Ditto.

> Source/WebCore/css/CSSStyleApplyProperty.cpp:756
> +	   CSSValueListInspector inspector = value;

We should use constructor syntax here:
CSSValueListInspector inspector(value);

Also, it appears that this site is the only user of the CSSValueListInspector
class. Are there plans to expand its usage? Otherwise I think we could use
CSSValueList methods directly here and remove the class.

> Source/WebCore/css/CSSStyleApplyProperty.cpp:762
> +	       pageSizeType = PAGE_SIZE_RESOLVED;
> +	       if (!inspector.first()->isPrimitiveValue() ||
!inspector.second()->isPrimitiveValue())
> +		   return;

Nit: We should move the pageSizeType assignment to after the if statement to
avoid unnecessary work in the early-return case.

> Source/WebCore/css/CSSStyleApplyProperty.cpp:773
> +		   // The value order is guaranteed. See
CSSParser::parseSizeParameter.

Does this mean we could replace the primitive type checks in pageSizeFromName()
with assertions?

> Source/WebCore/css/CSSStyleSelector.h:339
>      Length mmLength(double mm) const;
>      Length inchLength(double inch) const;

These declarations should be removed now as well.