Bug 27160

Summary: Implement vw/vh/vm (viewport sizes) from CSS 3 Values and Units
Product: WebKit Reporter: Aryeh Gregor <ayg>
Component: CSSAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Enhancement CC: 7raivis, abarth, arv, bugmail, bugs.webkit.org, bwebb, chrisjshull, commit-queue, dglazkov, eoconnor, eric, florens, hyatt, jackalmage, jdv, joethomas, jwalden+bwo, kling, koivisto, macpherson, menard, mrmazda, Ms2ger, ojan, paulirish, peter, phiw2, rniwa, simon.fraser, tabatkins, tkent, webkit.bugzilla, webkit.org, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
URL: http://www.w3.org/TR/css3-values/#vw
Bug Depends on: 80316, 80897    
Bug Blocks: 82773, 82811, 83425    
Attachments:
Description Flags
Initial Version
eric: review-
testCase
none
Patch2
none
Patch-with-windowResize-Support
webkit.review.bot: commit-queue-
Patch-updated
none
Patch-updated-I
koivisto: review-
Patch-BasedOn-Antti's Review Comments
none
Patch-02-16
none
Patch-with-More-testCases
none
patch-Updated-TestCases
none
Patch-incorporated-Hyatt's-Review-Comments
none
Patch-With-Abstract-Interface
webkit.review.bot: commit-queue-
Archive of layout-test-results from ec2-cr-linux-03
none
Patch-With-Abstract-Interface-Updated
none
Patch-Updated
webkit.review.bot: commit-queue-
Archive of layout-test-results from ec2-cr-linux-03
none
Archive of layout-test-results from ec2-cr-linux-01
none
Patch none

Description Aryeh Gregor 2009-07-10 14:47:24 PDT
CSS 3 Values and Units specifies vw, vh, and vm units.  These are respectively proportional to the width of the viewport, the height of the viewport, and the minimum of the two.  These would be extremely useful for authors -- quite a few very nasty hacks could be retired if we could use these (like <http://www.alistapart.com/articles/fauxcolumns/> and <http://matthewjamestaylor.com/blog/equal-height-columns-cross-browser-css-no-hacks>).  dhyatt said in #webkit that I should file a bug asking for this, and that "they're very easy to implement".

As far as I know, no other rendering engine implements any of these units yet, but I could be mistaken.  I'm going to file a bug against Gecko in a minute as well.  See also this discussion on www-style:

http://lists.w3.org/Archives/Public/www-style/2009Jul/0021.html
Comment 1 Aryeh Gregor 2009-07-11 19:20:07 PDT
The Mozilla bug is:

https://bugzilla.mozilla.org/show_bug.cgi?id=503720
Comment 2 Chris J. Shull 2011-08-21 14:35:34 PDT
IE9 has these: http://msdn.microsoft.com/en-us/ie/ff468705.aspx#_CSS3_Values_Units
Comment 3 Ben Webb 2011-10-03 11:30:42 PDT
I've been working with the win8 dev preview and I'm waiting to see support for the new units in a real (non-ie) browser. If I had the foggiest idea how I would contribute support myself, but sadly I don't. Is anyone working on this problem?
Comment 4 Joe Thomas 2012-01-05 11:35:44 PST
I am working on this.
Comment 5 Ben Webb 2012-01-05 11:38:21 PST
Joe you're my hero.
Comment 6 Joe Thomas 2012-01-06 16:17:04 PST
Created attachment 121518 [details]
Initial Version
Comment 7 Joe Thomas 2012-01-06 16:21:17 PST
Created attachment 121519 [details]
testCase

Test Case for vw and vh unit which I found in https://bugzilla.mozilla.org/show_bug.cgi?id=503720
Comment 8 Peter Beverloo 2012-01-09 03:54:36 PST
The editor's draft talks about "vmin" rather than "vm", which you omitted from the patch. Recalling previous discussion on the topic, at first an alternative using calc() and min() was being considered, but ISSUE-190 (http://www.w3.org/Style/CSS/Tracker/issues/190) has been resolved by renaming the unit to "vmin". Is the omission deliberate?

http://dev.w3.org/csswg/css3-values/#viewport-relative-lengths
Comment 9 Joe Thomas 2012-01-09 12:11:34 PST
(In reply to comment #8)
> The editor's draft talks about "vmin" rather than "vm", which you omitted from the patch. Recalling previous discussion on the topic, at first an alternative using calc() and min() was being considered, but ISSUE-190 (http://www.w3.org/Style/CSS/Tracker/issues/190) has been resolved by renaming the unit to "vmin". Is the omission deliberate?
> 
> http://dev.w3.org/csswg/css3-values/#viewport-relative-lengths

I omitted vm deliberately based on http://www.w3.org/TR/css3-values/#viewport-relative-lengths. I can add the support during the next iteration after the patch review.

Can someone please review the proposed patch for vw/vh implementation?
Comment 10 Eric Seidel (no email) 2012-01-09 14:00:16 PST
Comment on attachment 121518 [details]
Initial Version

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

> Source/WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

Need tests :(
Comment 11 Eric Seidel (no email) 2012-01-09 14:00:32 PST
Also, your ChangeLog shoudl explain your change in greater detail.
Comment 12 Joe Thomas 2012-01-11 16:39:19 PST
Created attachment 122128 [details]
Patch2

Added new test case. Also more details to ChangeLog.
Comment 13 Joe Thomas 2012-01-23 14:04:07 PST
Can someone please review this?  Thanks.
Comment 14 Luke Macpherson 2012-01-26 20:26:06 PST
What happens when the window is resized? It seems like we need to re-calculate all lengths that have already been computed, which in effect means re-applying all CSS properties continuously during a window resize (ie. for every new width which the window size is changing). That seems like it would be really computationally expensive.

Code style wise, it would be nice to not have to add another parameter to computeLengthDouble and friends. Perhaps this is an opportunity to clean up that code by providing some more useful parameter that can provide the values necessary for the computation.
Comment 15 Tab Atkins 2012-01-27 07:43:10 PST
(In reply to comment #14)
> What happens when the window is resized? It seems like we need to re-calculate all lengths that have already been computed, which in effect means re-applying all CSS properties continuously during a window resize (ie. for every new width which the window size is changing). That seems like it would be really computationally expensive.

It's basically identical to using a lot of percentages, except it can potentially apply more widely.
Comment 16 Ben Webb 2012-01-27 07:49:09 PST
(In reply to comment #15)
> (In reply to comment #14)
> > What happens when the window is resized? It seems like we need to re-calculate all lengths that have already been computed, which in effect means re-applying all CSS properties continuously during a window resize (ie. for every new width which the window size is changing). That seems like it would be really computationally expensive.
> 
> It's basically identical to using a lot of percentages, except it can potentially apply more widely.

This is how it works in the current IE10 implementation.
Comment 17 Joe Thomas 2012-01-27 15:09:09 PST
(In reply to comment #16)
> (In reply to comment #15)
> > (In reply to comment #14)
> > > What happens when the window is resized? It seems like we need to re-calculate all lengths that have already been computed, which in effect means re-applying all CSS properties continuously during a window resize (ie. for every new width which the window size is changing). That seems like it would be really computationally expensive.
> > 
> > It's basically identical to using a lot of percentages, except it can potentially apply more widely.
> 
> This is how it works in the current IE10 implementation.

Thanks for the inputs. I will make a new patch which works with window resize.
Comment 18 Joe Thomas 2012-01-27 15:12:21 PST
Comment on attachment 122128 [details]
Patch2

vw vh are relative units and it needs to be updated with window resize which is missing in the current patch.
Comment 19 Luke Macpherson 2012-01-30 17:02:01 PST
(In reply to comment #15)
> (In reply to comment #14)
> > What happens when the window is resized? It seems like we need to re-calculate all lengths that have already been computed, which in effect means re-applying all CSS properties continuously during a window resize (ie. for every new width which the window size is changing). That seems like it would be really computationally expensive.
> 
> It's basically identical to using a lot of percentages, except it can potentially apply more widely.

That isn't true if you look at the implementation in the patch. Normally percentage lengths are not converted to pixel lengths while applying the CSS values to RenderStyle as they are here. Instead there is a special kind of LengthType for percentages. I think you'd need to add another length type similar to percent for viewport-relative lengths to make this work efficiently.
Comment 20 Joe Thomas 2012-01-30 17:42:26 PST
> That isn't true if you look at the implementation in the patch. Normally percentage lengths are not converted to pixel lengths while applying the CSS values to RenderStyle as they are here. Instead there is a special kind of LengthType for percentages. I think you'd need to add another length type similar to percent for viewport-relative lengths to make this work efficiently.

Yes, current patch does not do that. In order to distinguish between relative viewport-width and relative viewport-height, I guess we need to add two different LengthTypes(since length does not have any info about CSSPrimitive type). Can we do it with one?

Also, what would be the best way to get viewport size in Length Structure to convert the relative length to pixel length ?
Comment 21 Joe Thomas 2012-01-31 07:18:50 PST
(In reply to comment #20)

> Yes, current patch does not do that. In order to distinguish between relative viewport-width and relative viewport-height, I guess we need to add two different LengthTypes(since length does not have any info about CSSPrimitive type). Can we do it with one?

One new LengthType should be sufficient. And a new sub-category of relative viewport Lengths can be added.
> 
> Also, what would be the best way to get viewport size in Length Structure to convert the relative length to pixel length?

A new function argument can be added to the getter functions in Length. But that does not look nice. Any other better approach?
Comment 22 Joe Thomas 2012-02-07 18:43:34 PST
Created attachment 125981 [details]
Patch-with-windowResize-Support

This patch includes support for window-resizing(the unit value changes if the window gets resized.)
Comment 23 WebKit Review Bot 2012-02-07 20:46:14 PST
Comment on attachment 125981 [details]
Patch-with-windowResize-Support

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

New failing tests:
transforms/cssmatrix-3d-interface.xhtml
svg/dynamic-updates/SVG-dynamic-css-transform.html
transforms/svg-vs-css.xhtml
svg/dom/css-transforms.xhtml
transforms/cssmatrix-2d-interface.xhtml
Comment 24 WebKit Review Bot 2012-02-07 21:34:52 PST
Comment on attachment 125981 [details]
Patch-with-windowResize-Support

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

New failing tests:
transforms/cssmatrix-3d-interface.xhtml
svg/dynamic-updates/SVG-dynamic-css-transform.html
transforms/svg-vs-css.xhtml
svg/dom/css-transforms.xhtml
transforms/cssmatrix-2d-interface.xhtml
Comment 25 Joe Thomas 2012-02-07 22:49:41 PST
Created attachment 126010 [details]
Patch-updated

Fixed the failing tests
Comment 26 Luke Macpherson 2012-02-08 03:29:48 PST
Comment on attachment 126010 [details]
Patch-updated

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

> Source/WebCore/css/CSSStyleSelector.cpp:2379
> +static Length convertToLength(CSSPrimitiveValue* primitiveValue, CSSStyleSelector* styleSelector, bool toFloat, double multiplier = 1, bool *ok = 0)

This code should no longer be static, and should instead be a member function of CSSStyleSelector.

> Source/WebCore/css/CSSStyleSelector.cpp:2411
> +static Length convertToIntLength(CSSPrimitiveValue* primitiveValue, CSSStyleSelector* styleSelector, double multiplier = 1, bool *ok = 0)

This code should no longer be static, and should instead be a member function of CSSStyleSelector.

> Source/WebCore/css/CSSStyleSelector.cpp:2416
> +static Length convertToFloatLength(CSSPrimitiveValue* primitiveValue, CSSStyleSelector* styleSelector, double multiplier = 1, bool *ok = 0)

This code should no longer be static, and should instead be a member function of CSSStyleSelector.

> Source/WebCore/css/CSSStyleSelector.cpp:5692
> +Length RelativeviewportLength(CSSPrimitiveValue* primitiveValue, Document* document)

This code almost certainly belongs in CSSPrimitiveValue::computeLength.

> Source/WebCore/css/CSSStyleSelector.h:479
> +Length RelativeviewportLength(CSSPrimitiveValue*, Document*);

captialization

> Source/WebCore/platform/Length.h:248
> +    RelativeViewportLengthType relativeViewportLengthType() const { return static_cast<RelativeViewportLengthType>(m_viewportLengthType); }

I would like to s/RelativeViewport/ViewportRelative throughout this patch.
Comment 27 Luke Macpherson 2012-02-08 03:36:52 PST
Comment on attachment 126010 [details]
Patch-updated

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

> Source/WebCore/css/CSSStyleSelector.cpp:3606
> +        createTransformOperations(value, this, operations);

I think after you patch "this" is the only value passed in. In that case this parameter should be removed.
Comment 28 Joe Thomas 2012-02-08 15:08:42 PST
(In reply to comment #26)
> (From update of attachment 126010 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=126010&action=review
> 
Thanks for the review.

> > Source/WebCore/css/CSSStyleSelector.cpp:2379
> > +static Length convertToLength(CSSPrimitiveValue* primitiveValue, CSSStyleSelector* styleSelector, bool toFloat, double multiplier = 1, bool *ok = 0)
> 
> This code should no longer be static, and should instead be a member function of CSSStyleSelector.
> 
> > Source/WebCore/css/CSSStyleSelector.cpp:2411
> > +static Length convertToIntLength(CSSPrimitiveValue* primitiveValue, CSSStyleSelector* styleSelector, double multiplier = 1, bool *ok = 0)
> 
> This code should no longer be static, and should instead be a member function of CSSStyleSelector.
> 
> > Source/WebCore/css/CSSStyleSelector.cpp:2416
> > +static Length convertToFloatLength(CSSPrimitiveValue* primitiveValue, CSSStyleSelector* styleSelector, double multiplier = 1, bool *ok = 0)
> 
> This code should no longer be static, and should instead be a member function of CSSStyleSelector.
> 
I will make above 3 functions as member functions. I changed the function signature of createTransformOperations static member function to include CSSStyleSelector. I think it is a good idea to remove the static from here too. Please let me know your opinion on this.

> > Source/WebCore/css/CSSStyleSelector.cpp:5692
> > +Length RelativeviewportLength(CSSPrimitiveValue* primitiveValue, Document* document)
> 
> This code almost certainly belongs in CSSPrimitiveValue::computeLength.

I have not done this for 2 reasons. (1) CSSPrimitiveValue::ComputeLength is used mainly for "Fixed" length types for computing the actual length in pixels. It is not used for other Length types like "Percentage" or "Number". I think we should handle RelativeViewport length similar to "Percentage" since we are not calculating the pixel value in CSSPrimitiveValue class.  (2) RelativeViewport Length type needs a way to fetch the viewport size (currently I introduced an overloaded constructor and passing Document as an argument). So if I modify the arguments of CSSPrimitiveValue::computeLength templatized function, then I need to make changes in lots of places wherever this function is called. 

So I think it is better to keep it separate.
> 
> > Source/WebCore/css/CSSStyleSelector.h:479
> > +Length RelativeviewportLength(CSSPrimitiveValue*, Document*);
> 
> captialization

I will make this change.
> 
> > Source/WebCore/platform/Length.h:248
> > +    RelativeViewportLengthType relativeViewportLengthType() const { return static_cast<RelativeViewportLengthType>(m_viewportLengthType); }
> 
> I would like to s/RelativeViewport/ViewportRelative throughout this patch.

OK. I will change RelativeViewport to ViewportRelative everywhere.
Comment 29 Joe Thomas 2012-02-08 15:11:35 PST
(In reply to comment #27)
> (From update of attachment 126010 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=126010&action=review
> 
> > Source/WebCore/css/CSSStyleSelector.cpp:3606
> > +        createTransformOperations(value, this, operations);
> 
> I think after you patch "this" is the only value passed in. In that case this parameter should be removed.

I didn't understand this. Could you please elaborate?
Comment 30 Luke Macpherson 2012-02-09 00:48:20 PST
(In reply to comment #28)
> (In reply to comment #26)
> > (From update of attachment 126010 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=126010&action=review
> > 
> Thanks for the review.
> 
> > > Source/WebCore/css/CSSStyleSelector.cpp:2379
> > > +static Length convertToLength(CSSPrimitiveValue* primitiveValue, CSSStyleSelector* styleSelector, bool toFloat, double multiplier = 1, bool *ok = 0)
> > 
> > This code should no longer be static, and should instead be a member function of CSSStyleSelector.
> > 
> > > Source/WebCore/css/CSSStyleSelector.cpp:2411
> > > +static Length convertToIntLength(CSSPrimitiveValue* primitiveValue, CSSStyleSelector* styleSelector, double multiplier = 1, bool *ok = 0)
> > 
> > This code should no longer be static, and should instead be a member function of CSSStyleSelector.
> > 
> > > Source/WebCore/css/CSSStyleSelector.cpp:2416
> > > +static Length convertToFloatLength(CSSPrimitiveValue* primitiveValue, CSSStyleSelector* styleSelector, double multiplier = 1, bool *ok = 0)
> > 
> > This code should no longer be static, and should instead be a member function of CSSStyleSelector.
> > 
> I will make above 3 functions as member functions. I changed the function signature of createTransformOperations static member function to include CSSStyleSelector. I think it is a good idea to remove the static from here too. Please let me know your opinion on this.

Yes, that sounds good to me.

> > > Source/WebCore/css/CSSStyleSelector.cpp:5692
> > > +Length RelativeviewportLength(CSSPrimitiveValue* primitiveValue, Document* document)
> > 
> > This code almost certainly belongs in CSSPrimitiveValue::computeLength.
> 
> I have not done this for 2 reasons. (1) CSSPrimitiveValue::ComputeLength is used mainly for "Fixed" length types for computing the actual length in pixels. It is not used for other Length types like "Percentage" or "Number". I think we should handle RelativeViewport length similar to "Percentage" since we are not calculating the pixel value in CSSPrimitiveValue class.  (2) RelativeViewport Length type needs a way to fetch the viewport size (currently I introduced an overloaded constructor and passing Document as an argument). So if I modify the arguments of CSSPrimitiveValue::computeLength templatized function, then I need to make changes in lots of places wherever this function is called. 

That is a fair argument. I personally think that percentage and number-based lengths should be in computeLength too, but perhaps that is something to look at later on.

> So I think it is better to keep it separate.
> > 
> > > Source/WebCore/css/CSSStyleSelector.h:479
> > > +Length RelativeviewportLength(CSSPrimitiveValue*, Document*);
> > 
> > captialization
> 
> I will make this change.
> > 
> > > Source/WebCore/platform/Length.h:248
> > > +    RelativeViewportLengthType relativeViewportLengthType() const { return static_cast<RelativeViewportLengthType>(m_viewportLengthType); }
> > 
> > I would like to s/RelativeViewport/ViewportRelative throughout this patch.
> 
> OK. I will change RelativeViewport to ViewportRelative everywhere.
Comment 31 Joe Thomas 2012-02-09 14:46:22 PST
(In reply to comment #30)
> (In reply to comment #28)
> > (In reply to comment #26)
> > > (From update of attachment 126010 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=126010&action=review
> > > 
> > Thanks for the review.
> > 
> > > > Source/WebCore/css/CSSStyleSelector.cpp:2379
> > > > +static Length convertToLength(CSSPrimitiveValue* primitiveValue, CSSStyleSelector* styleSelector, bool toFloat, double multiplier = 1, bool *ok = 0)
> > > 
> > > This code should no longer be static, and should instead be a member function of CSSStyleSelector.
> > > 
> > > > Source/WebCore/css/CSSStyleSelector.cpp:2411
> > > > +static Length convertToIntLength(CSSPrimitiveValue* primitiveValue, CSSStyleSelector* styleSelector, double multiplier = 1, bool *ok = 0)
> > > 
> > > This code should no longer be static, and should instead be a member function of CSSStyleSelector.
> > > 
> > > > Source/WebCore/css/CSSStyleSelector.cpp:2416
> > > > +static Length convertToFloatLength(CSSPrimitiveValue* primitiveValue, CSSStyleSelector* styleSelector, double multiplier = 1, bool *ok = 0)
> > > 
> > > This code should no longer be static, and should instead be a member function of CSSStyleSelector.
> > > 
> > I will make above 3 functions as member functions. I changed the function signature of createTransformOperations static member function to include CSSStyleSelector. I think it is a good idea to remove the static from here too. Please let me know your opinion on this.
> 
> Yes, that sounds good to me.
> 
It's slightly difficult to make these functions as non-static member function of CSSStyleSelector. 

CSSStyleSelector::createTransformOperations is called from WebKitCSSMatrix::setMatrixValue and this class does not have access to CSSStyleSelector. It will be difficult to access this function if we make it non-static. It just passes null for style and rootElementStyle while calling CSSStyleSelector::createTransformOperations. 

convertToFloatLength & convertToIntLength are called from CSSStyleSelector::createTransformOperations. Since CSSStyleSelector argument is passed as NULL to createTransformOperations when called from WebKitCSSMatrix, it is not possible to call convertToFloatLength & convertToIntLength if they are member functions.
Comment 32 Joe Thomas 2012-02-09 15:14:48 PST
Created attachment 126385 [details]
Patch-updated-I

Review comments incorporated
Comment 33 Antti Koivisto 2012-02-11 12:20:22 PST
Comment on attachment 126385 [details]
Patch-updated-I

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

Someone more familiar with these standard should review (smfr, hyatt?).

> Source/WebCore/css/CSSStyleSelector.cpp:2433
> -static Length convertToLength(CSSPrimitiveValue* primitiveValue, RenderStyle* style, RenderStyle* rootStyle, bool toFloat, double multiplier = 1, bool *ok = 0)
> +static Length convertToLength(CSSPrimitiveValue* primitiveValue, CSSStyleSelector* styleSelector, bool toFloat, double multiplier = 1, bool *ok = 0)

I don't like passing CSSStyleSelector*. Just try passing the minimal information you actually need for this (viewport?)
Comment 34 Antti Koivisto 2012-02-11 12:33:46 PST
Comment on attachment 126385 [details]
Patch-updated-I

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

> Source/WebCore/platform/Length.h:308
> +    unsigned char m_viewportLengthType;
> +    Document *m_document;

You can't add this stuff to Length. It is a memory-critical basic type.
Comment 35 Antti Koivisto 2012-02-11 12:39:51 PST
Not to mention Lengths may get shared between documents for various reasons and putting Document pointer there is nonsensical.
Comment 36 Joe Thomas 2012-02-15 14:35:31 PST
Created attachment 127238 [details]
Patch-BasedOn-Antti's Review Comments

Patch based on Antti Koivisto's review comments

Included viewportSize as one of the parameter in the getter functions of Length structure (calcValue, calcMinValue, calcFloatValue) with a default value IntSize().

Currently, the viewportSize parameter is used only when the width and height is calculated in RenderBox. Length getters are used extensively in webCore. I think it is better to include the viewport parameter in other places on a need basis. Any thoughts on this?
Comment 37 Luke Macpherson 2012-02-15 17:00:46 PST
Comment on attachment 127238 [details]
Patch-BasedOn-Antti's Review Comments

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

This patch is looking a lot better to me. My only concern right now is that there appear to be a lot of places where calcValue/calcMinValue should be able to handle viewport units, but the viewport size is not being passed in. You would probably pick this up if you were using more interesting test cases. I suggest running through "grep -r calcValue\( Source/WebCore/" and seeing all the other ways that Lengths are used. There are also probably existing test cases that could be modified simply by adding the appropriate units, and that could save you some work.

> Source/WebCore/css/CSSStyleSelector.cpp:5768
> +Length viewportRelativeLength(CSSPrimitiveValue* primitiveValue)

Now that this has been so effectively simplified, could this be made a method on CSSPrimitiveValue? I'm trying to kill the CSSPrimitiveValue::primitiveType() method at the moment (it should currently only be used in SVG code), and would also like to merge all CSSPrimitiveValue to Length conversions into a single point soon.

> Source/WebCore/platform/Length.h:248
> +        return type() == ViewportRelativeWidth || type() == ViewportRelativeHeight || type() == ViewportRelativeMin; 

This can simply be return type() >= ViewportRelativeWidth;

> Source/WebCore/rendering/RenderBox.cpp:1753
> +        logicalWidthResult = computeBorderBoxLogicalWidth(logicalWidth.calcValue(availableLogicalWidth, document()->viewportSize()));

Is these the only place that viewport based units are used? I see 125 references to calcValue() in the codebase - why can't all of them potentially be viewport based?

> Source/WebCore/rendering/RenderBoxModelObject.cpp:1078
> +    LayoutUnit xPosition = fillLayer->xPosition().calcMinValue(positioningAreaSize.width() - geometry.tileSize().width(), IntSize(), true);

Can fillLayer->xPosition() return a viewport relative size? If you need to assume this I think you should add an assertion.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:1084
> +    LayoutUnit yPosition = fillLayer->yPosition().calcMinValue(positioningAreaSize.height() - geometry.tileSize().height(), IntSize(), true);

Can fillLayer->yPosition() return a viewport relative size? If you need to assume this I think you should add an assertion.
Comment 38 Antti Koivisto 2012-02-15 20:24:38 PST
Looks good to me, please just address Luke's comments.
Comment 39 Joe Thomas 2012-02-15 22:40:26 PST
(In reply to comment #37)
> (From update of attachment 127238 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=127238&action=review
> 
> This patch is looking a lot better to me. My only concern right now is that there appear to be a lot of places where calcValue/calcMinValue should be able to handle viewport units, but the viewport size is not being passed in. You would probably pick this up if you were using more interesting test cases. I suggest running through "grep -r calcValue\( Source/WebCore/" and seeing all the other ways that Lengths are used. There are also probably existing test cases that could be modified simply by adding the appropriate units, and that could save you some work.
> 
Yes, viewport relative units can be used in most of the places where we call calcValue/CalcMinValue. I will make that change, but I am afraid that the patch is going to be big. 

> > Source/WebCore/css/CSSStyleSelector.cpp:5768
> > +Length viewportRelativeLength(CSSPrimitiveValue* primitiveValue)
> 
> Now that this has been so effectively simplified, could this be made a method on CSSPrimitiveValue? I'm trying to kill the CSSPrimitiveValue::primitiveType() method at the moment (it should currently only be used in SVG code), and would also like to merge all CSSPrimitiveValue to Length conversions into a single point soon.
> 
I will move this function to CSSPrimitiveValue

> > Source/WebCore/platform/Length.h:248
> > +        return type() == ViewportRelativeWidth || type() == ViewportRelativeHeight || type() == ViewportRelativeMin; 
> 
> This can simply be return type() >= ViewportRelativeWidth;
> 
OK.

> > Source/WebCore/rendering/RenderBox.cpp:1753
> > +        logicalWidthResult = computeBorderBoxLogicalWidth(logicalWidth.calcValue(availableLogicalWidth, document()->viewportSize()));
> 
> Is these the only place that viewport based units are used? I see 125 references to calcValue() in the codebase - why can't all of them potentially be viewport based?
> 
I will make the change.

> > Source/WebCore/rendering/RenderBoxModelObject.cpp:1078
> > +    LayoutUnit xPosition = fillLayer->xPosition().calcMinValue(positioningAreaSize.width() - geometry.tileSize().width(), IntSize(), true);
> 
> Can fillLayer->xPosition() return a viewport relative size? If you need to assume this I think you should add an assertion.
> 
> > Source/WebCore/rendering/RenderBoxModelObject.cpp:1084
> > +    LayoutUnit yPosition = fillLayer->yPosition().calcMinValue(positioningAreaSize.height() - geometry.tileSize().height(), IntSize(), true);
> 
> Can fillLayer->yPosition() return a viewport relative size? If you need to assume this I think you should add an assertion.
Comment 40 WebKit Review Bot 2012-02-16 11:52:02 PST
Attachment 127238 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
First, rewinding head to replay your work on top of it...
Applying: dataTransfer.types (HTML5 drag & drop) should return DOMStringList
Using index info to reconstruct a base tree...
<stdin>:84: trailing whitespace.
        
<stdin>:333: trailing whitespace.
<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"
<stdin>:334: trailing whitespace.
 width="300" height="300" onload="runRepaintTest()">
<stdin>:335: trailing whitespace.
<script xlink:href="../../fast/repaint/resources/repaint.js" />
<stdin>:336: trailing whitespace.
<script><![CDATA[
warning: squelched 16 whitespace errors
warning: 21 lines add whitespace errors.
Falling back to patching base and 3-way merge...
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging LayoutTests/platform/chromium/test_expectations.txt
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 dataTransfer.types (HTML5 drag & drop) should return DOMStringList

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 41 Joe Thomas 2012-02-16 14:12:18 PST
Created attachment 127439 [details]
Patch-02-16

Patch which uses viewport relative units more widely
Comment 42 Antti Koivisto 2012-02-16 16:56:52 PST
The test coverage seems weak.
Comment 43 Luke Macpherson 2012-02-16 17:06:11 PST
Code LGTM. I hope Document::viewportSize() is fast (it probably is), because it's going to be hotter than necessary - could avoid that by passing document and only calling it when evaluating a viewport based unit. Agree that more test coverage is needed. Sorry, I know that's the least fun part of adding a feature like this.
Comment 44 Tab Atkins Jr. 2012-02-16 17:13:43 PST
Yes, ideal test coverage would be at least one usage of all three units in every property that can take lengths.  Easy to auto-generate.
Comment 45 Joe Thomas 2012-02-17 10:25:29 PST
OK. I will add more test cases. Tab, Could you please give me more details on how to auto-generate the test case?
Comment 46 Joe Thomas 2012-02-20 12:45:16 PST
While adding more test cases, I found out that some of the CSS properties does not work well with newly introduced units. I will investigate more on it and update the patch soon.
Comment 47 Joe Thomas 2012-02-21 17:22:44 PST
Created attachment 128091 [details]
Patch-with-More-testCases

Patch with more test cases added.
Comment 48 Joe Thomas 2012-02-22 17:19:36 PST
Created attachment 128343 [details]
patch-Updated-TestCases

Added more test cases.
Comment 49 Joe Thomas 2012-02-27 23:19:04 PST
Antti, Could you please review the patch? I added more test cases as per your review comments.
Comment 50 Antti Koivisto 2012-03-01 08:51:36 PST
(In reply to comment #46)
> While adding more test cases, I found out that some of the CSS properties does not work well with newly introduced units. I will investigate more on it and update the patch soon.

What was the problem? How did you address it?
Comment 51 Joe Thomas 2012-03-01 08:58:09 PST
(In reply to comment #50)
> (In reply to comment #46)
> > While adding more test cases, I found out that some of the CSS properties does not work well with newly introduced units. I will investigate more on it and update the patch soon.
> 
> What was the problem? How did you address it?

Viewport Size was not passed to calcValue function in some of the cases (line-height, margin, etc). I added support for those missing cases.
Comment 52 Joe Thomas 2012-03-01 10:30:49 PST
Hyatt, Antti Koivisto wanted you also to take a look at the rendering changes as per the IRC conversation. Patch looks good to him.

Could you please review the changes?
Comment 53 Dave Hyatt 2012-03-02 11:26:37 PST
Comment on attachment 128343 [details]
patch-Updated-TestCases

I think you should add a viewportSize() member to RenderObject that calls document()->viewportSize(). The sheer # of times I see document()->viewportSize() makes me think it's worth it.
Comment 54 Joe Thomas 2012-03-02 19:25:36 PST
Created attachment 129988 [details]
Patch-incorporated-Hyatt's-Review-Comments

Incorporated Hyatt's Review comments
Comment 55 Joe Thomas 2012-03-02 19:26:56 PST
(In reply to comment #53)
> (From update of attachment 128343 [details])
> I think you should add a viewportSize() member to RenderObject that calls document()->viewportSize(). The sheer # of times I see document()->viewportSize() makes me think it's worth it.

Done. Uploaded new patch.
Comment 56 Antti Koivisto 2012-03-02 23:09:35 PST
r=me
Comment 57 WebKit Review Bot 2012-03-03 00:35:53 PST
Comment on attachment 129988 [details]
Patch-incorporated-Hyatt's-Review-Comments

Clearing flags on attachment: 129988

Committed r109656: <http://trac.webkit.org/changeset/109656>
Comment 58 WebKit Review Bot 2012-03-03 00:36:03 PST
All reviewed patches have been landed.  Closing bug.
Comment 59 Andreas Kling 2012-03-03 03:30:25 PST
(In reply to comment #43)
> I hope Document::viewportSize() is fast (it probably is), because it's going to be hotter than necessary - could avoid that by passing document and only calling it when evaluating a viewport based unit.

Was this comment ever addressed? I have to agree with Luke here, it looks like we're adding a pointless function call to the common case where passing a Document* would suffice.
Comment 60 Antti Koivisto 2012-03-03 05:46:57 PST
Passing Document as-is to Length::calcValue is not an option as that would be a layering violation. Maybe an abstract interface? Or just take care that getting viewport is fast on all platforms.
Comment 61 Erik Arvidsson 2012-03-05 10:35:25 PST
This seems to have regressed Parser/html5-full-render by about 10%

http://webkit-perf.appspot.com/graph.html#tests=[[3068,2001,32196]]&sel=1330759211690,1330784299690&displayrange=7&datatype=running
Comment 62 Antti Koivisto 2012-03-05 12:57:01 PST
This was rolled out in http://trac.webkit.org/changeset/109785
Comment 63 Joe Thomas 2012-03-10 09:31:01 PST
Created attachment 131175 [details]
Patch-With-Abstract-Interface

Added Abstract interface class for fetching the Viewport size.

I ran Parser/html5-full-render on Mac Snow-Leopard with webkit revision r110376 with and without the new patch and below are the values

Webkit rev 110376 - {16540.5ms, 16448ms, 16431ms, 16506.5ms, 16580ms}

Webkit rev 110376 with Patch - {16463ms, 16517.5ms, 16593ms, 16543ms, 16506ms}


The values looks more or less same in Mac SL but there is a variation of around 150-200 ms. Is it possible test the patch on performance bots for any possible regressions?
Comment 64 WebKit Review Bot 2012-03-11 06:37:27 PDT
Comment on attachment 131175 [details]
Patch-With-Abstract-Interface

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

New failing tests:
dom/svg/level3/xpath/Comment_Nodes.svg
http/tests/misc/object-embedding-svg-delayed-size-negotiation.xhtml
css2.1/20110323/background-intrinsic-004.htm
css2.1/20110323/background-intrinsic-006.htm
css2.1/20110323/background-intrinsic-005.htm
compositing/images/direct-svg-image.html
css3/images/cross-fade-overflow-position.html
css2.1/20110323/replaced-intrinsic-002.htm
dom/svg/level3/xpath/Attribute_Nodes_xmlns.svg
http/tests/inspector/inspect-element.html
http/tests/misc/SVGFont-delayed-load.html
accessibility/aria-disabled.html
css2.1/20110323/replaced-intrinsic-005.htm
css2.1/20110323/replaced-intrinsic-004.htm
css2.1/20110323/replaced-intrinsic-003.htm
css2.1/20110323/background-intrinsic-002.htm
dom/svg/level3/xpath/Conformance_ID.svg
css2.1/20110323/background-intrinsic-007.htm
dom/svg/level3/xpath/Conformance_hasFeature_3.svg
dom/svg/level3/xpath/Conformance_hasFeature_empty.svg
css2.1/20110323/background-intrinsic-009.htm
compositing/masks/multiple-masks.html
css2.1/20110323/background-intrinsic-001.htm
css3/zoom-coords.xhtml
css2.1/20110323/replaced-intrinsic-001.htm
dom/svg/level3/xpath/Attribute_Nodes.svg
css2.1/20110323/background-intrinsic-008.htm
css3/images/cross-fade-invalidation.html
dom/svg/level3/xpath/Conformance_Expressions.svg
css2.1/20110323/background-intrinsic-003.htm
Comment 65 WebKit Review Bot 2012-03-11 06:38:01 PDT
Created attachment 131221 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 66 Joe Thomas 2012-03-11 09:31:48 PDT
Created attachment 131229 [details]
Patch-With-Abstract-Interface-Updated

Fixed the chromium test failures.

Parser/html5-full-render gives almost the same values with and without the patch on Mac Snow-Leopard as mentioned in comment #63. Is there a way to test the patch on perf bots on other platforms?
Comment 67 Ryosuke Niwa 2012-03-11 11:27:31 PDT
(In reply to comment #66)
> Parser/html5-full-render gives almost the same values with and without the patch on Mac Snow-Leopard as mentioned in comment #63. Is there a way to test the patch on perf bots on other platforms?

There's no EWS bot for performance tests. You need to land the patch and see what happens.
Comment 68 Antti Koivisto 2012-03-12 08:49:31 PDT
Looking at this patch, I think what we really want to do is to move calc*Value functions out from Length (and platform). That will remove the need to have clumsy virtual interface just to avoid the layering violation.

So instead of something like

return offset + style()->top().calcValue(containingBlock->availableHeight(), view());

you would have

return offset + calculateValue(style()->top(), containingBlock->availableHeight(), view());

where calculateValue() is defined in WebCore/css/LengthFunctions.h/.cpp (or similar).

It would be good to do this refactoring first, separate from the viewport size feature. How does this sound?
Comment 69 Joe Thomas 2012-03-12 15:15:29 PDT
(In reply to comment #68)
> Looking at this patch, I think what we really want to do is to move calc*Value functions out from Length (and platform). That will remove the need to have clumsy virtual interface just to avoid the layering violation.
> 
> So instead of something like
> 
> return offset + style()->top().calcValue(containingBlock->availableHeight(), view());
> 
> you would have
> 
> return offset + calculateValue(style()->top(), containingBlock->availableHeight(), view());
> 
> where calculateValue() is defined in WebCore/css/LengthFunctions.h/.cpp (or similar).
> 
> It would be good to do this refactoring first, separate from the viewport size feature. How does this sound?

I think it is a good idea. Bug 80897 is created for this.
Comment 70 Joe Thomas 2012-03-25 23:35:19 PDT
Created attachment 133734 [details]
Patch-Updated

Ran PerformanceTests/Parser/html5-full-render.html(which got regressed previously) with and without the patch and there is no performance difference on MAC SL.
Comment 71 WebKit Review Bot 2012-03-26 00:42:06 PDT
Comment on attachment 133734 [details]
Patch-Updated

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

New failing tests:
css3/viewport-relative-lengths/css3-viewport-relative-lengths-vw.html
Comment 72 WebKit Review Bot 2012-03-26 00:42:17 PDT
Created attachment 133743 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 73 WebKit Review Bot 2012-03-26 01:50:42 PDT
Comment on attachment 133734 [details]
Patch-Updated

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

New failing tests:
css3/viewport-relative-lengths/css3-viewport-relative-lengths-vw.html
Comment 74 WebKit Review Bot 2012-03-26 01:50:52 PDT
Created attachment 133750 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 75 Joe Thomas 2012-03-26 09:59:34 PDT
(In reply to comment #73)
> (From update of attachment 133734 [details])
> Attachment 133734 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/12131718
> 
> New failing tests:
> css3/viewport-relative-lengths/css3-viewport-relative-lengths-vw.html

Looks like there is a 1 or 2 pixel offset in displaying the text "TEST PASSED" in chromium bot which fails the ref test. This test passes on Mac.
Comment 76 Joe Thomas 2012-03-26 17:17:05 PDT
(In reply to comment #75)
> > 
> > New failing tests:
> > css3/viewport-relative-lengths/css3-viewport-relative-lengths-vw.html
> 
> Looks like there is a 1 or 2 pixel offset in displaying the text "TEST PASSED" in chromium bot which fails the ref test. This test passes on Mac.

Scrollbar width was not considered while getting the viewport size during length calculation which caused the mismatch. I will update the patch.
Comment 77 Joe Thomas 2012-03-27 08:13:17 PDT
Created attachment 134068 [details]
Patch

Previously failed test case should fix by this patch.
Comment 78 Antti Koivisto 2012-03-27 11:27:51 PDT
Comment on attachment 134068 [details]
Patch

r=me
Comment 79 Antti Koivisto 2012-03-27 11:28:33 PDT
r=me
Comment 80 WebKit Review Bot 2012-03-27 12:20:23 PDT
Comment on attachment 134068 [details]
Patch

Clearing flags on attachment: 134068

Committed r112301: <http://trac.webkit.org/changeset/112301>
Comment 81 WebKit Review Bot 2012-03-27 12:21:24 PDT
All reviewed patches have been landed.  Closing bug.
Comment 82 bugs.webkit.org 2013-02-12 07:59:10 PST
font-size specified with vw, vh, or vmin not adjusting on browser resize -- see https://bugs.webkit.org/show_bug.cgi?id=106035