Bug 16662 – CSS3: Implement calc()

• Component: CSS
• Status: RESOLVED FIXED

[David Smith]

This should be extremely useful to web authors.

[David Smith]

Created attachment 23963 [details]
Very early prototype

This is wrong in all kinds of exciting ways, but it gets far enough to correctly size the box in this screenshot: http://dscoder.com/calcboxsize.png

[David Smith]

Created attachment 24014 [details]
Makes %s work, and fixes a number of other things

This now has TODO comments all over it indicating places I'm unsure on. Styles like width:-webkit-calc(100% / 2); work now, where before it would get both the division by 2 and the % width wrong.

[Shinichiro Hamaji]

Created attachment 47600 [details]
full patch

[Shinichiro Hamaji]

Let me explain this patch. First of all, this patch is not small. I'm happy to split this into several chunks, but I'd like to post this patch first and get a chance to receive high level suggestions.

This patch implements CSS3's calc function (http://www.w3.org/TR/2006/WD-css3-values-20060919/#calc). For now, it is enabled only for width and height and the name of function is -webkit-calc, not calc.

For the lexer, I added dimident because ident accepts '-' and for 10px-5px, the current lexer considers "px-5px" is the dimension of "10".

As for parser, I didn't reuse the current parser for other functions and added calc_function target because the kinds of arguments calc functions take are different from other functions.

Changes for Length class would be the most tricky part of this patch. Because Length objects have only 32bits (a single integer) and we don't want to increase the size of this objects, I embedded the ID of a calc length in the Length object. The ID starts from 0, increase to 0x0fffffff (4bits are for type of length), and return to 0 and increase again. The actual data of calc length can be obtained via hashmap whose key is the ID and value is the pointer to the data. As Length is copied by value, the data of calc length is reference-counted so we can delete them when the length becomes unnecessary.

This patch will be complained by check-webkit-style in CSSParser.cpp. I'm not sure if I should fix this. I guess check-webkit-style is too strict here.

If high level design of this patch sounds, I'll start creating some patches which refactor code needed to implement calc. Any kind of comments will be really appreciated.

Thanks David for your work. Your patch gave me a good start point.

[WebKit Review Bot]

Attachment 47600 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/css/CSSParser.cpp:1019:  One space before end of line comments  [whitespace/comments] [5]
WebCore/css/CSSParser.cpp:1020:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 2


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 47600 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/216612

[Eric Seidel (no email)]

Looks like this would break Chromium:
/usr/bin/ld: out/Release/obj.target/../../WebCore/WebCore.gyp/libwebcore.a(CSSParser.o): in function WebCore::CSSParser::parseCalculatedLength(int, WebCore::CSSParserValue*, bool):CSSParser.cpp(.text._ZN7WebCore9CSSParser21parseCalculatedLengthEiPNS_14CSSParserValueEb+0x2a): error: undefined reference to 'WebCore::CSSCalcValue::createFromParserValueList(WebCore::CSSParserValueList*)'
collect2: ld returned 1 exit status

[Eric Seidel (no email)]

Comment on attachment 47600 [details]
full patch

r- for the chromium build break.

[Shinichiro Hamaji]

Created attachment 48285 [details]
full patch v2

[WebKit Review Bot]

Attachment 48285 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/css/CSSParser.cpp:1019:  One space before end of line comments  [whitespace/comments] [5]
WebCore/css/CSSParser.cpp:1020:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 2


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 48285 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/240305

[Shinichiro Hamaji]

> Attachment 48285 [details] did not build on chromium:
> Build output: http://webkit-commit-queue.appspot.com/results/240305

I'm not sure if this failure is related to my patch. The error message:

g++: Internal error: Killed (program cc1plus)
Please submit a full bug report.
See <file:///usr/share/doc/gcc-4.4/README.Bugs> for instructions.
make: *** [out/Release/obj.target/webcore/../../WebCore/svg/SVGAllInOne.o] Error 1

[David Smith]

Who would be the appropriate reviewer for this area? I'd do it myself but, I'm not a reviewer. I'd love to see this make progress.

[Sam Weinig]

Comment on attachment 48285 [details]
full patch v2

The changes to Length.h/cpp are layering violation.  The intent of the platform layer is that it has no knowledge of the classes such as those in the dom, css and rendering directories.  This violation needs to be fixed before going forward.

r-.

[Shinichiro Hamaji]

Created attachment 48748 [details]
full patch v3

[WebKit Review Bot]

Attachment 48748 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/css/CSSParser.cpp:1019:  One space before end of line comments  [whitespace/comments] [5]
WebCore/css/CSSParser.cpp:1020:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 2


If any of these errors are false positives, please file a bug against check-webkit-style.

[Shinichiro Hamaji]

> The changes to Length.h/cpp are layering violation.  The intent of the platform
> layer is that it has no knowledge of the classes such as those in the dom, css
> and rendering directories.  This violation needs to be fixed before going
> forward.

Thanks for your review! I updated my patch and we don't have dependencies from platform/ to css/ anymore. I created a new file CalcValue.h/cpp. CalcValue is created by CSSCalcValue with RenderStyle objects. RenderStyle objects are necessary to resolve EMS and REMS. As EMS and REMS have been already resolved in CalcValue, we don't need dependencies to CSS from this class.

[WebKit Review Bot]

Attachment 48748 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/268548

[WebKit Review Bot]

Attachment 48748 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/268550

[Shinichiro Hamaji]

Created attachment 48752 [details]
full patch v4

[WebKit Review Bot]

Attachment 48752 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/css/CSSParser.cpp:1019:  One space before end of line comments  [whitespace/comments] [5]
WebCore/css/CSSParser.cpp:1020:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 2


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Barth]

Comment on attachment 48752 [details]
full patch v4

Holy cow.  This sounds pretty cool, but I'm sorry that we aren't able to accept this patch at this time.  This patch has been sitting in the review queue for a month and no one seems willing or able to review the patch.  Instead of just letting the patch linger here forever, I'm marking the patch r- (with the lame excuse of a style violation).

My guess is the best way to proceed here is to
(1) contact a reviewer who knows this part of the code,
(2) make sure we want to implement this feature at this time,
(3) get some feedback your approach, and
(4) have the reviewer review your code.

The patch, as it stands, raise the following questions for me:
(A) Are we sure we want to abuse the higher order bits of Length to track this extra state information?  That doesn't seem to scale well to future code and non 32-bit architectures.
(B) Why are we implementing this as -webkit-calc?  If the spec is ready to be implemented, it seems like we should just implement as calc and save the web the trouble of dealing with our vendor-specific non-sense.
(C) Is there a CSS3 conformance test in this area we can use to test our implementation?

I apologize for the delay in reviewing your patch, but it's important to communicate with other members of the project before going off and implementing features.

[David Smith]

I've talked with dhyatt and others about this a number of times on irc, both when I wrote the initial prototype patch, and to try to get some reviewer attention subsequently when it was moved forward again by Shinichiro Hamaji. Some of these issues have already been hashed out. I don't really understand why this is being r-'d, or why it's being suggested that it's been done without any communication with other project members...

If memory serves, Mozilla's bug on this is currently hung up on potential spec changes; if that's the case then *that* seems like a possibly valid reason for an r-.

[Adam Barth]

> I've talked with dhyatt and others about this a number of times on irc, both
> when I wrote the initial prototype patch, and to try to get some reviewer
> attention subsequently when it was moved forward again by Shinichiro Hamaji.

I don't see any comments from dhyatt on this bug.

> Some of these issues have already been hashed out.

That might well be true, but it's impossible to tell that from this bug.

> I don't really understand
> why this is being r-'d, or why it's being suggested that it's been done without
> any communication with other project members...

I've r-'d the patch because it's been sitting in the review queue for over a month with no action and no evidence whether we want this feature at this time.  Somewhere the process broke down with this patch.  We can either let it linger forever in the pending-review pile or try to make progress here.  I'd like to see us make progress.

Contributing code to the project is more than just writing a patch and throwing it over the wall.  It's important to work with the community and generate support for your patch.  It sounds like you've done some of that, but somewhere along the way, that process broke down and this patch got left behind.

[William J. Edney]

All -

Every time I discuss this feature with other web developers I always get positive feedback, so I consider this feature to be "extremely useful".

The Mozilla guys have decided to implement this as '-moz-calc()', but I believe they're hewing close to the spec itself, so it could probably just be 'calc()':

https://bugzilla.mozilla.org/show_bug.cgi?id=363249

Also, IE9 Preview #3 has now added support for the 'calc()' feature.

I'm just sayin... ;-)

Cheers,

- Bill

[Jon Rimmer]

So, I found found this bug through it being result #2 on Google for 'webkit calc'. I was thinking of using calc in some CSS for a site I'm developing, and knowing that Firefox and IE9 have it, I'd figured it'd almost certainly be in Webkit.

Imagine my delight therefore, upon discovering that actually Webkit doesn't support it, despite the fact that someone SUBMITTED A PATCH FOR IT 6 MONTHS AGO. It seems the collective developer-power of Apple, Google and however-many other contributors Webkit has, cannot muster the effort required to review a submission of an extremely useful new feature?

Great work guys! Keep it up!

[William J. Edney]

Jon -

I'm going to disagree with your direction here. Snarky comments rarely achieve the objective - I know from personal experience. There are real people sitting on the other end of the connection here who have lots on their plate, including real lives, and while I totally agree that I would *love* to have this patch approved and checked in, this isn't going to get us anywhere.

I have friends who work at both Apple and Google and, while they are big organizations, the resources aren't unlimited. Especially Webkit-knowledgable resources.

Yes, it is frustrating to wait. But I have Mozilla Bugzilla bugs that have submitted patches that have waited several *years*, so 6 months is quite short ;-).

Please be patient. I'm sure the Webkit guys know how useful this is. They'll get to it when they can.

Cheers,

- Bill

[Mike Lawther]

Created attachment 81518 [details]
Patch

[Mike Lawther]

Here is another attempt at getting CSS calc into WebKit :)

This patch implements CSS3 calc(), min() and max() as in the CSS3 Values and Units Editor's Draft 27 August 2010 http://dev.w3.org/csswg/css3-values/ (direct link at http://www.w3.org/TR/2010/ED-css3-values-20100827 appears broken :( ).

I do not intend to land this patch in one hunk - but in the smallest pieces I can (eg just the lexing stage, then the parsing stage etc). I've uploaded the whole patch so as to show how the whole thing fits together. I know it's not completely polished (that can happen in child bugs).

Thanks to David Smith and Shinichiro Hamaji - this patch builds on your good work.

[Shinichiro Hamaji]

(In reply to comment #29)
> Here is another attempt at getting CSS calc into WebKit :)

Great! I'm very excited to see this work is moving forward again. As Adam suggested me, I think we should discuss high level design of this patch on webkit-dev. Especially, I'm not sure if people like the change for Length, I couldn't come up with better idea though.

Ah, I've just noticed I didn't reply to Adam, sorry. Thanks Adam for your suggestions. My team decided to work on other project after I wrote the patch so I couldn't re-start this work so far.

[Mike Lawther]

Dave Hyatt did run an eye over the patch when I first uploaded it. He made some suggestions on IRC (eg use RefCounted for CalculatedLength - which I've already done but not uploaded yet). He said he had some more comments which he'd put in this bug, so I'm waiting on those to see what he reckons.

I got some advice from other reviewers to simply upload a smaller patch for review - which I have done at 54412.

[Mike Lawther]

Created attachment 86133 [details]
Current WIP

[Mike Lawther]

Yes - I'm still working on this :)

Based on some IRC comments from Dave Hyatt, I've been making calc() work with more properties, as well as with tables. To this end, I've integrated calc more tightly with CSSPrimitiveValue, so most of the existing machinery and validation logic can be used with minimal modification.

I'd welcome any feedback about this approach. It feels like a better one, since it enabled me to remove a bunch of special case code in CSSParser.cpp and CSSStyleSelector.cpp.

This is still a WIP, so some of the new tests I've written are failing (eg tables work for simple values, but fail for percents).

[Chris J. Shull]

Hey Mike (et al), as a web dev who's going to use the hell out of calc(), I just want to say thank you.

[Build Bot]

Attachment 86133 [details] did not build on win:
Build output: http://queues.webkit.org/results/8199049

[Mike Lawther]

Created attachment 86745 [details]
version after lexer/grammar patch has landed

[Build Bot]

Attachment 86745 [details] did not build on win:
Build output: http://queues.webkit.org/results/8231778

[Eric Seidel (no email)]

Comment on attachment 86745 [details]
version after lexer/grammar patch has landed

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

We're going to deal with this patch in pieces on other bugs.

> Source/WebCore/css/CSSCalcValue.cpp:85
> +    PassOwnPtr<CalcValue> toCalcValue(RenderStyle* style, RenderStyle* rootStyle, double zoom) const

What is this used by?  Seems odd.

> Source/WebCore/css/CSSStyleSelector.cpp:4345
> +                OwnPtr<CalcValue> calcValue = primitiveValue->getCalcValue()->toCalcValue(style(), m_rootElementStyle, zoomFactor);

WebCore avoids "get" in function names, normally.

> Source/WebCore/platform/CalcValue.cpp:43
> +    ASSERT(op == '+' || op == '-' || op == '*' || op == '/' || op == '%');

This goes away with an enum.

> Source/WebCore/platform/CalcValue.cpp:50
> +    switch (m_operator) {

Yeah, just an enum for m_operator and then the compiler helps you! :)

> Source/WebCore/platform/CalcValue.cpp:103
> +    if (m_type == MIN) {
> +        retval = m_list[0]->evaluate(maxValue);
> +        for (unsigned i = 1; i < m_list.size(); ++i) {
> +            float current = m_list[i]->evaluate(maxValue);
> +            if (current < retval)
> +                retval = current;
> +        }
> +    } else {
> +        retval = m_list[0]->evaluate(maxValue);
> +        for (unsigned i = 1; i < m_list.size(); ++i) {
> +            float current = m_list[i]->evaluate(maxValue);
> +            if (current > retval)
> +                retval = current;
> +        }
> +    }

I'm not sure what this copy/paste code is trying to do.

> Source/WebCore/platform/CalcValue.h:48
> +    enum UnitCategory {
> +        UNumber,
> +        UPercent,
> +        ULength,
> +        UOther
> +    };

I might still pull this enum out, even though there is a bunch of other things in this class.

> Source/WebCore/platform/CalcValue.h:80
> +    virtual float evaluate(float maxValue) const

What's maxValues?

> Source/WebCore/platform/CalcValue.h:101
> +class CalcMinMaxValueList : public CalcValue {

It's possible that we coudl avoid having to expose all this CalcValue stuff to platform, if we could design a system whereby we pre-process a calc tree down to a Length in the form: ax + by + c where x and y are containing block width and font size, respecitively.  Not sure if such is possible, but might be worth thinking about.

> Source/WebCore/platform/Length.cpp:255
> +float Length::calculatedCalcFloatValue(int maxValue) const

If you go down this hash road, I might put i in a separate class.  A class which Length can have a static member of.  Or some function in length.cpp has it as a static function local.

[Andrey Romanov]

This patch for webkit would be very useful for the web project I am working at (and probably for other web developers as well). Are there chances that it's gonna be applied in the next 6 months? (want to decide whether to write lots of additional javascript for this feature or not)

[Simon Fraser (smfr)]

<rdar://problem/10042910>

[Mike Lawther]

Created attachment 105572 [details]
Patch

[Mike Lawther]

Thanks very much for the review Eric - I'm sorry for the lengthy delay in getting back to you.

(In reply to comment #38)
> > Source/WebCore/css/CSSCalcValue.cpp:85
> > +    PassOwnPtr<CalcValue> toCalcValue(RenderStyle* style, RenderStyle* rootStyle, double zoom) const
> 
> What is this used by?  Seems odd.

This is used to convert from a CSSCalcValue to a CalcValue. There are two classes to prevent a layering violation between WebCore/css and WebCore/platform.

> > Source/WebCore/css/CSSStyleSelector.cpp:4345
> > +                OwnPtr<CalcValue> calcValue = primitiveValue->getCalcValue()->toCalcValue(style(), m_rootElementStyle, zoomFactor);
> 
> WebCore avoids "get" in function names, normally.

Done.

> > Source/WebCore/platform/CalcValue.cpp:43
> > +    ASSERT(op == '+' || op == '-' || op == '*' || op == '/' || op == '%');
> 
> This goes away with an enum.

Done.

> > Source/WebCore/platform/CalcValue.cpp:50
> > +    switch (m_operator) {
> 
> Yeah, just an enum for m_operator and then the compiler helps you! :)

Done.

> > Source/WebCore/platform/CalcValue.cpp:103
> > +    if (m_type == MIN) {
> > +        retval = m_list[0]->evaluate(maxValue);
> > +        for (unsigned i = 1; i < m_list.size(); ++i) {
> > +            float current = m_list[i]->evaluate(maxValue);
> > +            if (current < retval)
> > +                retval = current;
> > +        }
> > +    } else {
> > +        retval = m_list[0]->evaluate(maxValue);
> > +        for (unsigned i = 1; i < m_list.size(); ++i) {
> > +            float current = m_list[i]->evaluate(maxValue);
> > +            if (current > retval)
> > +                retval = current;
> > +        }
> > +    }
> 
> I'm not sure what this copy/paste code is trying to do.

These evaluate the min or max of a list. It is a little cut+pastey, but the alternative was to have the min/max test inside the loop, and since it was invariant I wanted to hoist it,

> > Source/WebCore/platform/CalcValue.h:48
> > +    enum UnitCategory {
> > +        UNumber,
> > +        UPercent,
> > +        ULength,
> > +        UOther
> > +    };
> 
> I might still pull this enum out, even though there is a bunch of other things in this class.

Done (and renamed).

> > Source/WebCore/platform/CalcValue.h:80
> > +    virtual float evaluate(float maxValue) const
> 
> What's maxValues?

It's the size of the containing box, used for percentage calculation. This argument name was reused from the existing functions that do this calculation.

> > Source/WebCore/platform/CalcValue.h:101
> > +class CalcMinMaxValueList : public CalcValue {
> 
> It's possible that we coudl avoid having to expose all this CalcValue stuff to platform, if we could design a system whereby we pre-process a calc tree down to a Length in the form: ax + by + c where x and y are containing block width and font size, respecitively.  Not sure if such is possible, but might be worth thinking about.

Hmm - that's a great idea. It's definitely possible to trivially simplify the calc expression tree if adjacent nodes can be combined etc. I had thought this could be done in a followup patch - do you reckon it should be attempted upfront?

> > Source/WebCore/platform/Length.cpp:255
> > +float Length::calculatedCalcFloatValue(int maxValue) const
> 
> If you go down this hash road, I might put i in a separate class.  A class which Length can have a static member of.  Or some function in length.cpp has it as a static function local.

Done.

Eric - I'd previously been splitting off what I thought were minimal chunks for separate review/landing. When you did the review, some things in the smaller patch (at https://bugs.webkit.org/show_bug.cgi?id=57082) were confusing without the full context. At this stage, would you prefer that I slice off the parsing stage for review again, or would you prefer to review everything  in context here?

[Gyuyoung Kim]

Comment on attachment 105572 [details]
Patch

Attachment 105572 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/9565329

[WebKit Review Bot]

Comment on attachment 105572 [details]
Patch

Attachment 105572 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9558492

New failing tests:
css3/calc/table-calcs.html

[Mike Lawther]

Created attachment 105581 [details]
added new files to EFL compile

[WebKit Review Bot]

Comment on attachment 105581 [details]
added new files to EFL compile

Attachment 105581 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9567318

New failing tests:
css3/calc/table-calcs.html

[Eric Seidel (no email)]

Comment on attachment 105581 [details]
added new files to EFL compile

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

I don't feel like I gave you a great review here.  Most of this has been purged from my memory by now.  There are a few style issues, and some possible leaks.  I'm not sure who is your best hope for reviewer here.  Your'e in sydney, right?  You might pick on Luke to get him to sanity check your CSS changes as he's been in that file as of late.  Ideally beth dakin would look at this, but it may be hard to get some of her time.

> Source/WebCore/css/CSSCalcValue.cpp:52
> +static CalcCategory unitCategory(CSSPrimitiveValue::UnitTypes type)

Don't we have a header with all these conversion functions to/from CSSPrimativeValue somewhere?

> Source/WebCore/css/CSSCalcValue.cpp:71
> +    default:
> +        return CalcOther;

Do we need this default?  Normally we try and avoid them to let the compiler warn when we forget a case.

> Source/WebCore/css/CSSCalcValue.cpp:79
> +    static PassRefPtr<CSSCalcPrimitiveValue> create(CSSPrimitiveValue* value, bool isInt)

Ick.  Is isInt normally part of the signature?  That really should be an enum.

> Source/WebCore/css/CSSCalcValue.cpp:86
> +        return !m_value->getDoubleValue();

Normally we don't have "get" in method names, but I suspect CSSPrimativeValue is just old (not your fault here, obviously).

Will this do the right thing for NaN?

> Source/WebCore/css/CSSCalcValue.cpp:94
> +    PassOwnPtr<CalcValue> toCalcValue(RenderStyle* style, RenderStyle* rootStyle, double zoom) const

Should this be a create function on CalcValue?  to* functions are normally static conversions that don't return Pass* pointers, but this is OK too.

> Source/WebCore/css/CSSCalcValue.cpp:110
> +        default:
> +            return nullptr;

Please no default:

> Source/WebCore/css/CSSCalcValue.cpp:120
> +    explicit CSSCalcPrimitiveValue(CSSPrimitiveValue* value, bool isInt)
> +        : m_value(value)                 
> +    {
> +        m_category = unitCategory((CSSPrimitiveValue::UnitTypes)value->primitiveType());
> +        m_isInt = isInt;
> +    }

Ah, so this is your cusomt signature.  Please make this isInt thing an enum.

> Source/WebCore/css/CSSCalcValue.cpp:163
> +        switch (op) {
> +        case '+':
> +        case '-':                
> +            if (leftCategory == rightCategory)    
> +                newCategory = leftCategory;
> +            else if (leftCategory != CalcNumber && rightCategory != CalcNumber)
> +                newCategory = CalcPercent;
> +            else    
> +                return 0;
> +            break;
> +                
> +        case '*':
> +            if (leftCategory != CalcNumber && rightCategory != CalcNumber) 
> +                return 0;
> +            
> +            if (leftCategory == CalcNumber)
> +                newCategory = rightCategory;
> +            else 
> +                newCategory = leftCategory;
> +            break;
> +
> +        case '/':
> +        case '%':
> +            if (rightCategory != CalcNumber || rightSide->isZero())
> +                return 0;
> +            newCategory = leftCategory;
> +            break;
> +
> +        default:
> +            ASSERT_NOT_REACHED();
> +            return 0;

This feels like a newCatagoryForOperator helper function, which you call, and then check that CalcCategory is a valid value?

> Source/WebCore/css/CSSCalcValue.cpp:177
> +        return "(" + m_leftSide->cssText() + m_operator + m_rightSide->cssText() + ")";

return makeString(foo, bar, baz) is better.

> Source/WebCore/css/CSSCalcValue.cpp:183
> +        if (!left.get())

You don't need .get(), there is a bool operator on OwnPtr already.

> Source/WebCore/css/CSSCalcValue.cpp:197
> +        ASSERT(op == '+' || op == '-' || op == '*' || op == '/' || op == '%');  

Should we make op an Enum instad?  WE could even give that enum the same values as these strings?  Unclear if that woudl be better or not. Certainly would make some of these signatures cleaner.

> Source/WebCore/css/CSSCalcValue.cpp:293
> +    struct Value {
> +        RefPtr<CSSCalcValue> value;
> +    };

I'm confused by why we have this struct.  It could also just be a type-def if you wanted to save typing RefPtr everywhere.

> Source/WebCore/css/CSSCalcValue.cpp:298
> +            return 0;

As an Enum this could return nice semantic enums, like InvalidOperator (which could equal 0).

> Source/WebCore/css/CSSCalcValue.h:73
> +    void setIsInt(bool isInt) { m_isInt = isInt; }

It's not immediately clear to me why this is externally mutable, but OK.

> Source/WebCore/css/CSSParser.cpp:702
> +        break;            

typo.

> Source/WebCore/css/CSSPrimitiveValue.cpp:105
> +    default:
> +        return CSSPrimitiveValue::CSS_UNKNOWN;

Can we move this out of the default case?  Again, we tend to avoid default: in WebCore where possible.

> Source/WebCore/css/CSSStyleApplyProperty.cpp:335
> +                  return;

indent.

> Source/WebCore/css/CSSStyleSelector.cpp:5223
> +    return *new Length(CSSPrimitiveValue::create(mm, CSSPrimitiveValue::CSS_MM)->computeLength<Length>(style(), m_rootElementStyle));

How does this ever get deleted?

> Source/WebCore/css/CSSStyleSelector.cpp:5228
> +    return *new Length(CSSPrimitiveValue::create(inch, CSSPrimitiveValue::CSS_IN)->computeLength<Length>(style(), m_rootElementStyle));

Leak?

> Source/WebCore/platform/CalcValue.cpp:59
> +    switch (op) {
> +    case '+':
> +        m_operator = CalcAdd;
> +        return;
> +    case '-':
> +        m_operator = CalcSubtract;
> +        return;
> +    case '*':
> +        m_operator = CalcMultiply;
> +        return;
> +    case '/':
> +        m_operator = CalcDivide;
> +        return;
> +    case '%':
> +        m_operator = CalcMod;
> +        return;
> +    }
> +    ASSERT_NOT_REACHED();

You want a helper function here.  m_operator =

> Source/WebCore/platform/Length.cpp:169
> +    : m_quirk(false), m_type(Calculated), m_isFloat(false)

one per line.

[Ryosuke Niwa]

How does this feature affect editing? e.g. can calc be used in font-size property?

[Mike Lawther]

The spec says "The calc(), min(), and max() functions can be used wherever <length>, <frequency>, <angle>, <time>, or <number> values are allowed."

So I read that as yes, this can be used in the font-size property, for example

  font-size: -webkit-calc(150% - 2px);

[Ryosuke Niwa]

(In reply to comment #49)
> The spec says "The calc(), min(), and max() functions can be used wherever <length>, <frequency>, <angle>, <time>, or <number> values are allowed."
> 
> So I read that as yes, this can be used in the font-size property, for example
> 
>   font-size: -webkit-calc(150% - 2px);

WebKit supports execCommand('fontSizeDelta', false, '5px'); for example. It seems like -webkit-calc will be incompatibile with this particular feature as it is implemented now. We also need to make sure functions like legacyFontSizeFromCSSValue and fontSizeForKeyword work correctly with -webkit-calc.

[Nicholas Shanks]

Mike, just out of curiosity, would your code work in background-position as a replacement for offsets only being permitted from the edges?

e.g. I am trying to badge icons in table cells. the icon behind is 48px, horizontally centered, 10px from the top.
The badge icon is 24px and should be overlaid in the bottom-right corner of the main icon, with a small overhang beyond the main icon's extent. Thus it needs something like background-position: calc(center + 16px) top 62px, top 10px;

This syntax isn't working in Opera, and I was wondering if I should apply your patch and try it in WebKit.

[Mike Lawther]

The 'center' syntax definitely won't work. I don't fully grok your example. but I think -webkit-calc(50% - (24px / 2) + 16px) is what you want.

[Mike Lawther]

Created attachment 118038 [details]
Patch

[Mike Lawther]

Comment on attachment 118038 [details]
Patch

This latest patch works on pretty much every property, and comes with lots of new test goodness.

It does not completely work on tables - however the spec now allows a UA in the presence of calc() in a table to act as though 'auto' has been specified ("calc() expressions [...] in both auto and fixed layout tables may be treated as if ‘auto’ had been specified"). The other place it does not completely work is for getComputedStyle.

These are arguably corner cases which I reckon can be addressed after landing the main functionality.

I know this patch is large, and I don't intend to land it as is. I plan to slice off smaller chunks and land those - but the full patch provides context.

I'd appreciate any reviewers comments on this full patch though.

[Mike Lawther]

Created attachment 118050 [details]
Patch

[Mike Lawther]

The parsing stage patch is up at https://bugs.webkit.org/show_bug.cgi?id=57082.

[Mike Lawther]

For those playing along at home:

http://trac.webkit.org/changeset/110148 has landed a major piece of calc functionality - expressions that mix absolute lengths and percentages (eg -webkit-calc(100% - 10px)).

There is still more work to do to enable calc for every single property (eg border-radius does not support calc yet), but calc works for width and height and that should be enough for people to start playing, testing and filing bugs :)

Please note that min() and max() are NOT supported - they have been removed from the latest draft of the spec (http://dev.w3.org/csswg/css3-values/#calc).

If you do file a bug, please prefix the bug description with 'CSS3 calc:' and cc me, so it's easy for me to find them.

[j.j.]

Mozilla will ship Firefox 16 with unprefixed calc() support.
https://bugzilla.mozilla.org/show_bug.cgi?id=771678
Microsoft supports calc() unprefixed only since IE9.

So I think WebKit should support unprefixed calc() as well.

[Ojan Vafai]

(In reply to comment #58)
> Mozilla will ship Firefox 16 with unprefixed calc() support.
> https://bugzilla.mozilla.org/show_bug.cgi?id=771678
> Microsoft supports calc() unprefixed only since IE9.
> 
> So I think WebKit should support unprefixed calc() as well.

In think unprefixing soon is reasonable. We should do an audit of our implementation compared to IE9/FF16 to see how compatible we are (e.g. run out calc tests in those browsers, see if Mozilla has calc tests we could run in WebKit).

[Mike Lawther]

Mozilla has unprefixed calc https://bugzilla.mozilla.org/show_bug.cgi?id=771678.

I've filed https://bugs.webkit.org/show_bug.cgi?id=91951 to track our unprefixing.

[Ojan Vafai]

Comment on attachment 118050 [details]
Patch

This patch is done right? As in, it's been committed in sub parts? Marking R- to remove from the queue. Obviously, feel free to upload something new if there's still more to do here.

[Mike Lawther]

Yes - this patch has been committed in parts.

There are still a couple of dependent bugs here which I'll close out before closing this bug.

[Mike Lawther]

As calc() support has been in behind a -webkit prefix for almost a year, and has recently been unprefixed, I think it's time to let go and close out this bug :)

There are some remaining implementation niggles as documented in the bugs linked off this one, but I don't think we need to keep this bug open for those.