Bug 16662 - CSS3: Implement calc()
Summary: CSS3: Implement calc()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Enhancement
Assignee: Mike Lawther
URL: http://www.w3.org/TR/css3-values/#calc
Keywords: InRadar
Depends on: 80357 107482 111149 54412 57082 75815 75818 75819 75822 75823 75893 75896 75898 75902 75998 76000 76005 76006 76130 76226 77960 78226 78321 78325 78347 78446 78603 79188 79621 80379 80411 80489 80813 81182 81477 81478 82017 82152 82161 84551 91951 94158 105985 106046 136026
Blocks: 46590
  Show dependency treegraph
 
Reported: 2007-12-29 00:52 PST by David Smith
Modified: 2014-08-17 11:37 PDT (History)
58 users (show)

See Also:


Attachments
Very early prototype (14.24 KB, patch)
2008-09-30 16:56 PDT, David Smith
no flags Details | Formatted Diff | Diff
Makes %s work, and fixes a number of other things (19.16 KB, patch)
2008-10-02 03:43 PDT, David Smith
no flags Details | Formatted Diff | Diff
full patch (59.15 KB, patch)
2010-01-28 02:22 PST, Shinichiro Hamaji
no flags Details | Formatted Diff | Diff
full patch v2 (59.55 KB, patch)
2010-02-05 21:05 PST, Shinichiro Hamaji
no flags Details | Formatted Diff | Diff
full patch v3 (68.18 KB, patch)
2010-02-15 06:07 PST, Shinichiro Hamaji
no flags Details | Formatted Diff | Diff
full patch v4 (68.16 KB, patch)
2010-02-15 07:37 PST, Shinichiro Hamaji
no flags Details | Formatted Diff | Diff
Patch (86.53 KB, patch)
2011-02-07 14:23 PST, Mike Lawther
no flags Details | Formatted Diff | Diff
Current WIP (125.65 KB, patch)
2011-03-17 21:01 PDT, Mike Lawther
no flags Details | Formatted Diff | Diff
version after lexer/grammar patch has landed (103.64 KB, patch)
2011-03-23 22:55 PDT, Mike Lawther
no flags Details | Formatted Diff | Diff
Patch (104.85 KB, patch)
2011-08-29 21:15 PDT, Mike Lawther
no flags Details | Formatted Diff | Diff
added new files to EFL compile (105.55 KB, patch)
2011-08-29 22:49 PDT, Mike Lawther
no flags Details | Formatted Diff | Diff
Patch (451.48 KB, patch)
2011-12-06 06:25 PST, Mike Lawther
no flags Details | Formatted Diff | Diff
Patch (447.97 KB, patch)
2011-12-06 07:43 PST, Mike Lawther
ojan: review-
mikelawther: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Smith 2007-12-29 00:52:15 PST
This should be extremely useful to web authors.
Comment 1 David Smith 2008-09-30 16:56:35 PDT
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
Comment 2 David Smith 2008-10-02 03:43:38 PDT
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.
Comment 3 Shinichiro Hamaji 2010-01-28 02:22:43 PST
Created attachment 47600 [details]
full patch
Comment 4 Shinichiro Hamaji 2010-01-28 02:23:33 PST
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.
Comment 5 WebKit Review Bot 2010-01-28 02:29:31 PST
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.
Comment 6 WebKit Review Bot 2010-01-28 03:02:17 PST
Attachment 47600 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/216612
Comment 7 Eric Seidel (no email) 2010-02-01 16:31:23 PST
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
Comment 8 Eric Seidel (no email) 2010-02-04 16:35:38 PST
Comment on attachment 47600 [details]
full patch

r- for the chromium build break.
Comment 9 Shinichiro Hamaji 2010-02-05 21:05:19 PST
Created attachment 48285 [details]
full patch v2
Comment 10 WebKit Review Bot 2010-02-05 21:09:04 PST
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.
Comment 11 WebKit Review Bot 2010-02-05 22:41:27 PST
Attachment 48285 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/240305
Comment 12 Shinichiro Hamaji 2010-02-05 22:48:30 PST
> 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
Comment 13 David Smith 2010-02-06 12:38:19 PST
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.
Comment 14 Sam Weinig 2010-02-12 14:07:30 PST
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-.
Comment 15 Shinichiro Hamaji 2010-02-15 06:07:29 PST
Created attachment 48748 [details]
full patch v3
Comment 16 WebKit Review Bot 2010-02-15 06:09:17 PST
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.
Comment 17 Shinichiro Hamaji 2010-02-15 06:12:39 PST
> 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.
Comment 18 WebKit Review Bot 2010-02-15 06:27:07 PST
Attachment 48748 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/268548
Comment 19 WebKit Review Bot 2010-02-15 06:28:04 PST
Attachment 48748 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/268550
Comment 20 Shinichiro Hamaji 2010-02-15 07:37:11 PST
Created attachment 48752 [details]
full patch v4
Comment 21 WebKit Review Bot 2010-02-15 07:42:51 PST
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.
Comment 22 Adam Barth 2010-03-22 08:39:24 PDT
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.
Comment 23 David Smith 2010-03-22 09:05:54 PDT
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-.
Comment 24 Adam Barth 2010-03-22 10:07:53 PDT
> 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.
Comment 25 William J. Edney 2010-06-24 04:29:43 PDT
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
Comment 26 Jon Rimmer 2010-07-11 10:05:43 PDT
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!
Comment 27 William J. Edney 2010-07-11 10:26:12 PDT
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
Comment 28 Mike Lawther 2011-02-07 14:23:25 PST
Created attachment 81518 [details]
Patch
Comment 29 Mike Lawther 2011-02-07 14:24:34 PST
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.
Comment 30 Shinichiro Hamaji 2011-02-08 22:12:15 PST
(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.
Comment 31 Mike Lawther 2011-02-14 14:33:54 PST
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.
Comment 32 Mike Lawther 2011-03-17 21:01:20 PDT
Created attachment 86133 [details]
Current WIP
Comment 33 Mike Lawther 2011-03-17 21:20:28 PDT
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).
Comment 34 Chris J. Shull 2011-03-17 21:42:50 PDT
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.
Comment 35 Build Bot 2011-03-17 22:03:58 PDT
Attachment 86133 [details] did not build on win:
Build output: http://queues.webkit.org/results/8199049
Comment 36 Mike Lawther 2011-03-23 22:55:22 PDT
Created attachment 86745 [details]
version after lexer/grammar patch has landed
Comment 37 Build Bot 2011-03-23 23:25:35 PDT
Attachment 86745 [details] did not build on win:
Build output: http://queues.webkit.org/results/8231778
Comment 38 Eric Seidel (no email) 2011-05-06 16:09:30 PDT
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.
Comment 39 Andrey Romanov 2011-08-28 13:09:29 PDT
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)
Comment 40 Simon Fraser (smfr) 2011-08-29 16:46:57 PDT
<rdar://problem/10042910>
Comment 41 Mike Lawther 2011-08-29 21:15:14 PDT
Created attachment 105572 [details]
Patch
Comment 42 Mike Lawther 2011-08-29 21:28:18 PDT
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?
Comment 43 Gyuyoung Kim 2011-08-29 21:38:32 PDT
Comment on attachment 105572 [details]
Patch

Attachment 105572 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/9565329
Comment 44 WebKit Review Bot 2011-08-29 22:04:50 PDT
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
Comment 45 Mike Lawther 2011-08-29 22:49:03 PDT
Created attachment 105581 [details]
added new files to EFL compile
Comment 46 WebKit Review Bot 2011-08-29 23:44:04 PDT
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
Comment 47 Eric Seidel (no email) 2011-08-30 15:12:40 PDT
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.
Comment 48 Ryosuke Niwa 2011-10-04 10:55:59 PDT
How does this feature affect editing? e.g. can calc be used in font-size property?
Comment 49 Mike Lawther 2011-10-05 19:24:24 PDT
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);
Comment 50 Ryosuke Niwa 2011-10-05 20:25:50 PDT
(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.
Comment 51 Nicholas Shanks 2011-10-06 07:08:04 PDT
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.
Comment 52 Mike Lawther 2011-10-09 15:51:47 PDT
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.
Comment 53 Mike Lawther 2011-12-06 06:25:35 PST
Created attachment 118038 [details]
Patch
Comment 54 Mike Lawther 2011-12-06 06:35:54 PST
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.
Comment 55 Mike Lawther 2011-12-06 07:43:45 PST
Created attachment 118050 [details]
Patch
Comment 56 Mike Lawther 2012-01-08 15:14:38 PST
The parsing stage patch is up at https://bugs.webkit.org/show_bug.cgi?id=57082.
Comment 57 Mike Lawther 2012-03-08 16:02:31 PST
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.
Comment 58 j.j. 2012-07-14 13:55:58 PDT
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.
Comment 59 Ojan Vafai 2012-07-18 14:20:14 PDT
(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).
Comment 60 Mike Lawther 2012-07-22 16:45:41 PDT
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.
Comment 61 Ojan Vafai 2012-08-15 18:59:39 PDT
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.
Comment 62 Mike Lawther 2012-08-16 17:07:39 PDT
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.
Comment 63 Mike Lawther 2013-02-18 22:36:03 PST
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.