Bug 79621 - CSS3 calc: mixed absolute/percentages work for width, height, margin and padding
Summary: CSS3 calc: mixed absolute/percentages work for width, height, margin and padding
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: 80558
Blocks: 16662
  Show dependency treegraph
 
Reported: 2012-02-26 21:26 PST by Mike Lawther
Modified: 2012-03-07 22:49 PST (History)
7 users (show)

See Also:


Attachments
Patch (43.11 KB, patch)
2012-02-26 21:36 PST, Mike Lawther
no flags Details | Formatted Diff | Diff
Patch (41.29 KB, patch)
2012-02-26 22:08 PST, Mike Lawther
no flags Details | Formatted Diff | Diff
Patch (41.04 KB, patch)
2012-02-27 20:13 PST, Mike Lawther
no flags Details | Formatted Diff | Diff
Patch (40.98 KB, patch)
2012-02-27 20:19 PST, Mike Lawther
no flags Details | Formatted Diff | Diff
Patch (31.82 KB, patch)
2012-03-06 06:34 PST, Mike Lawther
no flags Details | Formatted Diff | Diff
Patch (32.26 KB, patch)
2012-03-06 17:45 PST, Mike Lawther
no flags Details | Formatted Diff | Diff
Patch (31.92 KB, patch)
2012-03-06 20:30 PST, Mike Lawther
no flags Details | Formatted Diff | Diff
Patch (30.88 KB, patch)
2012-03-06 21:49 PST, Mike Lawther
no flags Details | Formatted Diff | Diff
Patch (31.12 KB, patch)
2012-03-06 22:16 PST, Mike Lawther
no flags Details | Formatted Diff | Diff
Patch for landing (31.15 KB, patch)
2012-03-07 15:45 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-26 21:26:47 PST
CSS3 calc: mixed absolute/percentages work for width, height, margin and padding
Comment 1 Mike Lawther 2012-02-26 21:36:04 PST
Created attachment 128948 [details]
Patch
Comment 2 Mike Lawther 2012-02-26 22:08:48 PST
Created attachment 128952 [details]
Patch
Comment 3 Ojan Vafai 2012-02-27 17:06:05 PST
Comment on attachment 128952 [details]
Patch

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

> Source/WebCore/css/CSSCalculationValue.cpp:136
> +            break;

What's the case where we hit this?

> Source/WebCore/platform/CalculationValue.cpp:78
> +    // Skip m_index == 0 as zero key hits the assertion of HashMap.

Make this ASSERT(m_index) instead. Then you don't need to check m_index in the loop below.

> Source/WebCore/platform/CalculationValue.cpp:82
> +    m_index++;

Why do you need to increment here? Won't the loop above already get you the right index value?

> Source/WebCore/platform/Length.cpp:225
> +    float result = calcLength->evaluate(maxValue);
> +    if (isnan(result))
> +        return 0;
> +    return result;

This is repeated three times. Maybe make this a static helper function? nonNanResult or something?

> Source/WebCore/platform/Length.h:159
> +    // Note: May only be called for Fixed, Percent, Auto and Calculated lengths.
>      // Other types will ASSERT in order to catch invalid length calculations.

This comment isn't terribly useful given that the code makes this pretty clear.

> Source/WebCore/platform/Length.h:245
> +    bool isPercent() const { return type() == Percent || type() == Calculated; }

Can't Calculated be a non-percent?
Comment 4 Ojan Vafai 2012-02-27 17:07:04 PST
This patch looks good to me. It's a bit tricky though, so I'd prefer someone with more experience with CSS review this. Antti, would you mind taking a look at this?
Comment 5 Mike Lawther 2012-02-27 20:11:17 PST
Comment on attachment 128952 [details]
Patch

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

>> Source/WebCore/css/CSSCalculationValue.cpp:136
>> +            break;
> 
> What's the case where we hit this?

Good catch - we should never hit that.

>> Source/WebCore/platform/CalculationValue.cpp:78
>> +    // Skip m_index == 0 as zero key hits the assertion of HashMap.
> 
> Make this ASSERT(m_index) instead. Then you don't need to check m_index in the loop below.

Fair enough - this just catches the extremely unlikely case of wraparound of the index.

>> Source/WebCore/platform/CalculationValue.cpp:82
>> +    m_index++;
> 
> Why do you need to increment here? Won't the loop above already get you the right index value?

m_index was supposed to be the next available index. So we find the next available index, use it, and step m_index along. The loop is really there for a sanity check.

But now that I think about it, it's probably better not to step m_index along - then we better deal with the case of insertion/removal of the same element (with nothing else happening inbetween) - m_index will not continually increment. Unit tests would be nice right about now :)

>> Source/WebCore/platform/Length.cpp:225
>> +    return result;
> 
> This is repeated three times. Maybe make this a static helper function? nonNanResult or something?

Done.

>> Source/WebCore/platform/Length.h:159
>>      // Other types will ASSERT in order to catch invalid length calculations.
> 
> This comment isn't terribly useful given that the code makes this pretty clear.

OK - removed.

>> Source/WebCore/platform/Length.h:245
>> +    bool isPercent() const { return type() == Percent || type() == Calculated; }
> 
> Can't Calculated be a non-percent?

If it gets this far, it has to be a mixed percent/absolute expression. If it's a simple length, percent or number, it gets evaluated inside CSSPrimitiveValue.
Comment 6 Mike Lawther 2012-02-27 20:13:30 PST
Created attachment 129174 [details]
Patch
Comment 7 Mike Lawther 2012-02-27 20:16:32 PST
Comment on attachment 129174 [details]
Patch

Thanks for the review Ojan! I've addressed your comments in this new patch.
Comment 8 Mike Lawther 2012-02-27 20:19:09 PST
Created attachment 129175 [details]
Patch
Comment 9 Mike Lawther 2012-02-27 20:20:31 PST
Comment on attachment 129175 [details]
Patch

The last patch didn't remove that comment. It's gone now.
Comment 10 Mike Lawther 2012-02-29 14:41:15 PST
+hyatt, since this touches platform/Length.[h|cpp].
Comment 11 Dimitri Glazkov (Google) 2012-03-05 12:43:14 PST
Comment on attachment 129175 [details]
Patch

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

A few suggestions, just trying to move the patch along.

Can this patch be made a little less massive by perhaps:
1) removing calculatedIs* stubs?
2) adding CalcExpression* incrementally?

I wonder if Hyatt and Antti will be more inclined to review in smaller bits?

> Source/WebCore/css/CSSStyleSelector.h:352
> +    static Length convertToIntLength(CSSPrimitiveValue*, RenderStyle*, RenderStyle* rootStyle, double multiplier = 1, bool *ok = 0);
> +    static Length convertToFloatLength(CSSPrimitiveValue*, RenderStyle*, RenderStyle* rootStyle, double multiplier = 1, bool *ok = 0);

These conversion functions (including the convertToLength itself) all look like the could be factored to live on the CSSPrimitiveValue.

> Source/WebCore/platform/CalculationValue.cpp:66
> +    // FIXME: result is NaN when there is a division by zero which

File a bug and reference it here?

> Source/WebCore/platform/CalculationValue.h:133
> +class HandleMap {

The name is way too generic to be in WebCore namespace. Can this just be an implementation detail in Length.cpp?

> Source/WebCore/platform/Length.cpp:179
> +// FIXME: It's impossible to know the sign of calc values without

File a bug and reference it here?

> Source/WebCore/platform/Length.cpp:196
> +bool Length::calculatedIsZero() const
> +{
> +    return false;
> +}
> +
> +bool Length::calculatedIsPositive() const
> +{
> +    return true;
> +}
> +
> +bool Length::calculatedIsNegative() const
> +{
> +    return false;
> +}

Why are these here?
Comment 12 Mike Lawther 2012-03-06 06:31:06 PST
Thanks for the review Dimitri.

> A few suggestions, just trying to move the patch along.
> 
> Can this patch be made a little less massive by perhaps:
> 1) removing calculatedIs* stubs?
> 2) adding CalcExpression* incrementally?

This patch is already pretty pared back - everything is kinda 'bare minimum to function'. 
 - I've removed those stubs and replaced them directly with the current return values.
 - Adding CalcExpression* incrementally could only be done non-functionally - ie I could put those bits in in separate patches, but nothing would 'work'.
 - I've landed the rewritten margin.html test separately.

> > Source/WebCore/css/CSSStyleSelector.h:352
> > +    static Length convertToIntLength(CSSPrimitiveValue*, RenderStyle*, RenderStyle* rootStyle, double multiplier = 1, bool *ok = 0);
> > +    static Length convertToFloatLength(CSSPrimitiveValue*, RenderStyle*, RenderStyle* rootStyle, double multiplier = 1, bool *ok = 0);
> 
> These conversion functions (including the convertToLength itself) all look like the could be factored to live on the CSSPrimitiveValue.

Fair point. I'm inclined to do this as a followup though, since those functions handle NULL CSSPrimitiveValue pointers, meaning the pointer would potentially have to be tested before calling a function on it. Or the NULL check is unnecessary/obsolete.

> > Source/WebCore/platform/CalculationValue.cpp:66
> > +    // FIXME: result is NaN when there is a division by zero which
> 
> File a bug and reference it here?

Done.

> > Source/WebCore/platform/CalculationValue.h:133
> > +class HandleMap {
> 
> The name is way too generic to be in WebCore namespace. Can this just be an implementation detail in Length.cpp?

Once upon a time it was in Length.cpp, but I pulled it out in response to some IRC comments from Hyatt. I've put it back - it does reduce the size of this patch marginally.

> > Source/WebCore/platform/Length.cpp:179
> > +// FIXME: It's impossible to know the sign of calc values without
> 
> File a bug and reference it here?

Done.

> > Source/WebCore/platform/Length.cpp:196
> > +bool Length::calculatedIsZero() const
> > +{
> > +    return false;
> > +}
> > +
> > +bool Length::calculatedIsPositive() const
> > +{
> > +    return true;
> > +}
> > +
> > +bool Length::calculatedIsNegative() const
> > +{
> > +    return false;
> > +}
> 
> Why are these here?

Just as explicit functions saying what they do. I've removed these and folded their return values directly into the callsites instead.
Comment 13 Mike Lawther 2012-03-06 06:34:40 PST
Created attachment 130371 [details]
Patch
Comment 14 Dimitri Glazkov (Google) 2012-03-06 10:04:18 PST
Comment on attachment 130371 [details]
Patch

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

> Source/WebCore/platform/Length.cpp:192
> +    HashMap<int, RefPtr<CalculationValue> >* m_map;

It doesn't look like you're ever destroying this map? I understand the class is only allocated as static, but still that seems like something we should avoid. Does it have to be a pointer? Can it just be a member?
Comment 15 Andreas Kling 2012-03-06 12:43:04 PST
Comment on attachment 130371 [details]
Patch

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

Some drive-by comments:

> Source/WebCore/css/CSSCalculationValue.cpp:135
> +        case CalcPercentNumber:
> +        case CalcOther:
> +            ASSERT_NOT_REACHED();

This seems like a good place for a comment.

> Source/WebCore/platform/Length.cpp:157
> +Length::~Length()
> +{
> +    if (isCalculated())
> +        decrementCalculatedRef();
> +}   

This destructor should be inline, only decrementCalculatedRef() belongs in Length.cpp.

> Source/WebCore/platform/Length.cpp:162
> +    : m_index(1)

Wrong indentation here.

> Source/WebCore/platform/Length.cpp:199
> +static CalculationValueHandleMap* calcHandles()
> +{
> +    DEFINE_STATIC_LOCAL(CalculationValueHandleMap, handleMap, ());
> +    return &handleMap;
> +}

Nit: static CalculationValueHandleMap& calcHandles() would be a slightly nicer signature.
Comment 16 Mike Lawther 2012-03-06 17:44:09 PST
Comment on attachment 130371 [details]
Patch

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

Thanks for the reviews guys! Comments inline ....

>> Source/WebCore/css/CSSCalculationValue.cpp:135
>> +            ASSERT_NOT_REACHED();
> 
> This seems like a good place for a comment.

Done.

>> Source/WebCore/platform/Length.cpp:157
>> +}   
> 
> This destructor should be inline, only decrementCalculatedRef() belongs in Length.cpp.

OK - done.

>> Source/WebCore/platform/Length.cpp:162
>> +    : m_index(1)
> 
> Wrong indentation here.

Fixed.

>> Source/WebCore/platform/Length.cpp:192
>> +    HashMap<int, RefPtr<CalculationValue> >* m_map;
> 
> It doesn't look like you're ever destroying this map? I understand the class is only allocated as static, but still that seems like something we should avoid. Does it have to be a pointer? Can it just be a member?

Good point. I've made it a member.

>> Source/WebCore/platform/Length.cpp:199
>> +}
> 
> Nit: static CalculationValueHandleMap& calcHandles() would be a slightly nicer signature.

I'm oldskool when it comes to pointers. But changed to a reference.
Comment 17 Mike Lawther 2012-03-06 17:45:15 PST
Created attachment 130498 [details]
Patch
Comment 18 Mike Lawther 2012-03-06 20:30:32 PST
Created attachment 130529 [details]
Patch
Comment 19 Mike Lawther 2012-03-06 21:49:37 PST
Created attachment 130541 [details]
Patch
Comment 20 Rafael Brandao 2012-03-06 21:53:24 PST
Comment on attachment 130529 [details]
Patch

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

> Source/WebCore/ChangeLog:13
> +        Length (in platform) cannot refer to CSSCalculatedValue (in css) due to layering

The terminology is a bit confusing, maybe I'm missing something. Along the patch you use CSSCalculationValue instead of of CalculatedValue, so which one will you use?

> Source/WebCore/platform/Length.cpp:164
> +            m_index++;

I believe at some point we could overflow this. Can't you reuse ids instead to avoid this?
Comment 21 Mike Lawther 2012-03-06 22:14:03 PST
Comment on attachment 130529 [details]
Patch

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

>> Source/WebCore/ChangeLog:13
>> +        Length (in platform) cannot refer to CSSCalculatedValue (in css) due to layering
> 
> The terminology is a bit confusing, maybe I'm missing something. Along the patch you use CSSCalculationValue instead of of CalculatedValue, so which one will you use?

My mistake, I meant [CSS]CalculationValue, not [CSS]CalculatedValue.

>> Source/WebCore/platform/Length.cpp:164
>> +            m_index++;
> 
> I believe at some point we could overflow this. Can't you reuse ids instead to avoid this?

It will overflow, after 2 billion or so indices, hence the ASSERT. Reusing ids would be more complicated than what's here.

I'd prefer to keep this patch as simple as possible until this is proven to be a problem. I've filed a bug to track this.
Comment 22 Mike Lawther 2012-03-06 22:16:36 PST
Created attachment 130543 [details]
Patch
Comment 23 Andreas Kling 2012-03-07 12:45:53 PST
Comment on attachment 130543 [details]
Patch

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

r=me based on prior partial approval from others, and the changes looking good to me.

It's great that you've sprinkled FIXMEs. Makes it much easier to understand the patch.

> Source/WebCore/css/CSSCalculationValue.cpp:129
> +            {
> +                Length length = CSSStyleSelector::convertToFloatLength(m_value.get(), style, rootStyle, zoom);
> +                return adoptPtr(new CalcExpressionLength(length));
> +            }

I'd write this as a single line.

> Source/WebCore/platform/Length.h:246
>      bool isZero() const 
>      {
>          ASSERT(!isUndefined());
> -        return m_isFloat ? !m_floatValue : !m_intValue;
> +        return isCalculated() ? false : m_isFloat ? !m_floatValue : !m_intValue;
> +    }
> +    bool isPositive() const
> +    {
> +        return isUndefined() ? false : isCalculated() ? true : getFloatValue() > 0;
> +    }
> +    bool isNegative() const
> +    {
> +        return isUndefined() ? false : isCalculated() ? false : getFloatValue() < 0;
>      }

I feel like we can do this without abusing ternary operators.

> Source/WebCore/platform/Length.h:315
> +    void incrementCalculatedRef() const;
> +    void decrementCalculatedRef() const;    

I don't understand why these are const.
Comment 24 Mike Lawther 2012-03-07 15:42:37 PST
Comment on attachment 130543 [details]
Patch

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

Yay - thanks for the review!

>> Source/WebCore/css/CSSCalculationValue.cpp:129
>> +            }
> 
> I'd write this as a single line.

Done.

>> Source/WebCore/platform/Length.h:246
>>      }
> 
> I feel like we can do this without abusing ternary operators.

OK - ternarys involving isCalculated removed.

>> Source/WebCore/platform/Length.h:315
>> +    void decrementCalculatedRef() const;    
> 
> I don't understand why these are const.

They will only affect the CalculationValueHandleMap, not the Length object itself. So Length can be const, as it is only holding a key into the map.
Comment 25 Mike Lawther 2012-03-07 15:45:08 PST
Created attachment 130712 [details]
Patch for landing
Comment 26 WebKit Review Bot 2012-03-07 18:12:43 PST
Comment on attachment 130712 [details]
Patch for landing

Clearing flags on attachment: 130712

Committed r110126: <http://trac.webkit.org/changeset/110126>
Comment 27 WebKit Review Bot 2012-03-07 18:12:50 PST
All reviewed patches have been landed.  Closing bug.
Comment 28 Mike Lawther 2012-03-07 21:24:42 PST
Reopening as the patch was rolled out due to causing a compile error due to exit time destructors: https://bugs.webkit.org/show_bug.cgi?id=80558.

Compile error fix has landed in http://trac.webkit.org/changeset/110144, so I'll try landing this patch again.
Comment 29 WebKit Review Bot 2012-03-07 22:48:53 PST
Comment on attachment 130712 [details]
Patch for landing

Clearing flags on attachment: 130712

Committed r110148: <http://trac.webkit.org/changeset/110148>
Comment 30 WebKit Review Bot 2012-03-07 22:49:00 PST
All reviewed patches have been landed.  Closing bug.