Bug 78446 - CSS3 calc: embed calc expressions in CSSPrimitiveValue
Summary: CSS3 calc: embed calc expressions in CSSPrimitiveValue
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mike Lawther
URL:
Keywords:
Depends on:
Blocks: 16662
  Show dependency treegraph
 
Reported: 2012-02-12 18:09 PST by Mike Lawther
Modified: 2012-02-14 01:47 PST (History)
3 users (show)

See Also:


Attachments
Patch (50.24 KB, patch)
2012-02-12 18:11 PST, Mike Lawther
no flags Details | Formatted Diff | Diff
Patch (51.00 KB, patch)
2012-02-13 16:56 PST, Mike Lawther
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mike Lawther 2012-02-12 18:09:21 PST
CSS3 calc: embed calc expressions in CSSPrimitiveValue
Comment 1 Mike Lawther 2012-02-12 18:11:35 PST
Created attachment 126697 [details]
Patch
Comment 2 Ojan Vafai 2012-02-13 15:18:34 PST
Comment on attachment 126697 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=126697&action=review

R- just for the multiplier issue.

> Source/WebCore/css/CSSCalculationValue.cpp:85
> +double CSSCalcValue::computeLengthPx(RenderStyle* currStyle, RenderStyle* rootStyle, double multiplier, bool computingFontSize) const

nit: s/currStyle/currentStyle. here and below

> Source/WebCore/css/CSSPrimitiveValue.cpp:59
> +    case CSSPrimitiveValue:: CSS_CALC_PERCENTAGE_NUMBER:
> +    case CSSPrimitiveValue:: CSS_CALC_PERCENTAGE_LENGTH:

I find these names a little confusing. It's a bit verbose, but how about CSS_CALC_PERCENTAGE_WITH_NUMBER and CSS_CALC_PERCENTAGE_WITH_LENGTH?

> Source/WebCore/css/CSSPrimitiveValue.cpp:485
> +    double value;

How about s/value/computedValue?

> Source/WebCore/css/CSSPrimitiveValue.cpp:487
> +        value = m_value.calc->computeLengthPx(style, rootStyle, 1.0, computingFontSize);

Should you not be passing multiplier instead of 1.0? If 1.0 is correct, then this at least needs a comment explaining why.

> Source/WebCore/css/CSSPrimitiveValue.h:141
> +        const unsigned short type = primitiveType();

I don't think the const buys you anything here, does it?
Comment 3 Mike Lawther 2012-02-13 16:54:53 PST
Comment on attachment 126697 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=126697&action=review

>> Source/WebCore/css/CSSPrimitiveValue.cpp:59
>> +    case CSSPrimitiveValue:: CSS_CALC_PERCENTAGE_LENGTH:
> 
> I find these names a little confusing. It's a bit verbose, but how about CSS_CALC_PERCENTAGE_WITH_NUMBER and CSS_CALC_PERCENTAGE_WITH_LENGTH?

Done.

>> Source/WebCore/css/CSSPrimitiveValue.cpp:485
>> +    double value;
> 
> How about s/value/computedValue?

Done.

>> Source/WebCore/css/CSSPrimitiveValue.cpp:487
>> +        value = m_value.calc->computeLengthPx(style, rootStyle, 1.0, computingFontSize);
> 
> Should you not be passing multiplier instead of 1.0? If 1.0 is correct, then this at least needs a comment explaining why.

I only want to apply the multiplier once. I could pass it in here and add extra logic to ensure I don't re-apply it down below - but I figured this was simpler. I've added a comment.

>> Source/WebCore/css/CSSPrimitiveValue.h:141
>> +        const unsigned short type = primitiveType();
> 
> I don't think the const buys you anything here, does it?

Not really. Force of habit trying to make everything const that is :) Removed.
Comment 4 Mike Lawther 2012-02-13 16:56:04 PST
Created attachment 126867 [details]
Patch
Comment 5 Mike Lawther 2012-02-13 16:56:51 PST
Comment on attachment 126867 [details]
Patch

Hey Ojan - thanks for the review! I've addressed your comments in this new patch.
Comment 6 WebKit Review Bot 2012-02-14 01:47:06 PST
Comment on attachment 126867 [details]
Patch

Clearing flags on attachment: 126867

Committed r107688: <http://trac.webkit.org/changeset/107688>
Comment 7 WebKit Review Bot 2012-02-14 01:47:11 PST
All reviewed patches have been landed.  Closing bug.