Bug 73000 – Implement CSSPropertySize in CSSStyleApplyProperty.

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Luke Macpherson]

Implement CSSPropertySize in CSSStyleApplyProperty.

[Luke Macpherson]

Created attachment 116311 [details]
Patch

[Andreas Kling]

Comment on attachment 116311 [details]
Patch

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.

[Luke Macpherson]

Comment on attachment 116311 [details]
Patch

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.

done.

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

done.

>> 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.

done.

>> Source/WebCore/css/CSSStyleApplyProperty.cpp:692
>> +            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.

Nice. I recall noticing this earlier but appear to forgotten to include that cleanup. Done.

>> Source/WebCore/css/CSSStyleApplyProperty.cpp:733
>> +                return false;
> 
> Ditto.

done.

>> 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.

Switched to constructor syntax. I think the rest of the discussion is beyond the scope of this refactoring though.

>> Source/WebCore/css/CSSStyleApplyProperty.cpp:762
>> +                return;
> 
> Nit: We should move the pageSizeType assignment to after the if statement to avoid unnecessary work in the early-return case.

I've moved it much further down.

>> 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?

Already removed.

>> Source/WebCore/css/CSSStyleSelector.h:339
>>      Length inchLength(double inch) const;
> 
> These declarations should be removed now as well.

done.

[Luke Macpherson]

Created attachment 116693 [details]
Patch

[Andreas Kling]

Comment on attachment 116693 [details]
Patch

r=me

[WebKit Review Bot]

Comment on attachment 116693 [details]
Patch

Rejecting attachment 116693 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
 succeeded at 1312 with fuzz 2 (offset 108 lines).
patching file Source/WebCore/css/CSSStyleSelector.cpp
Hunk #1 succeeded at 3626 (offset -132 lines).
Hunk #2 FAILED at 4021.
Hunk #3 succeeded at 3949 (offset -122 lines).
1 out of 3 hunks FAILED -- saving rejects to file Source/WebCore/css/CSSStyleSelector.cpp.rej
patching file Source/WebCore/css/CSSStyleSelector.h

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Andreas Kling', u'--fo..." exit_code: 1

Full output: http://queues.webkit.org/results/10668887

[Luke Macpherson]

Created attachment 116837 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 116837 [details]
Patch for landing

Clearing flags on attachment: 116837

Committed r101317: <http://trac.webkit.org/changeset/101317>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.