Bug 27160 - Implement vw/vh/vm (viewport sizes) from CSS 3 Values and Units
: Implement vw/vh/vm (viewport sizes) from CSS 3 Values and Units
Status: RESOLVED FIXED
: WebKit
CSS
: 528+ (Nightly build)
: All All
: P2 Enhancement
Assigned To:
: http://www.w3.org/TR/css3-values/#vw
:
: 80316 80897
: 82773 82811 83425
  Show dependency treegraph
 
Reported: 2009-07-10 14:47 PST by
Modified: 2013-07-16 06:18 PST (History)


Attachments
Initial Version (41.72 KB, patch)
2012-01-06 16:17 PST, Joe Thomas
eric: review-
Review Patch | Details | Formatted Diff | Diff
testCase (2.80 KB, application/xhtml+xml)
2012-01-06 16:21 PST, Joe Thomas
no flags Details
Patch2 (52.91 KB, patch)
2012-01-11 16:39 PST, Joe Thomas
no flags Review Patch | Details | Formatted Diff | Diff
Patch-with-windowResize-Support (48.98 KB, patch)
2012-02-07 18:43 PST, Joe Thomas
webkit.review.bot: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Patch-updated (48.41 KB, patch)
2012-02-07 22:49 PST, Joe Thomas
no flags Review Patch | Details | Formatted Diff | Diff
Patch-updated-I (48.46 KB, patch)
2012-02-09 15:14 PST, Joe Thomas
koivisto: review-
Review Patch | Details | Formatted Diff | Diff
Patch-BasedOn-Antti's Review Comments (37.53 KB, patch)
2012-02-15 14:35 PST, Joe Thomas
no flags Review Patch | Details | Formatted Diff | Diff
Patch-02-16 (105.72 KB, patch)
2012-02-16 14:12 PST, Joe Thomas
no flags Review Patch | Details | Formatted Diff | Diff
Patch-with-More-testCases (150.56 KB, patch)
2012-02-21 17:22 PST, Joe Thomas
no flags Review Patch | Details | Formatted Diff | Diff
patch-Updated-TestCases (167.25 KB, patch)
2012-02-22 17:19 PST, Joe Thomas
no flags Review Patch | Details | Formatted Diff | Diff
Patch-incorporated-Hyatt's-Review-Comments (166.65 KB, patch)
2012-03-02 19:25 PST, Joe Thomas
no flags Review Patch | Details | Formatted Diff | Diff
Patch-With-Abstract-Interface (179.38 KB, patch)
2012-03-10 09:31 PST, Joe Thomas
webkit.review.bot: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (19.46 MB, application/zip)
2012-03-11 06:38 PST, WebKit Review Bot
no flags Details
Patch-With-Abstract-Interface-Updated (179.36 KB, patch)
2012-03-11 09:31 PST, Joe Thomas
no flags Review Patch | Details | Formatted Diff | Diff
Patch-Updated (173.80 KB, patch)
2012-03-25 23:35 PST, Joe Thomas
webkit.review.bot: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (9.66 MB, application/zip)
2012-03-26 00:42 PST, WebKit Review Bot
no flags Details
Archive of layout-test-results from ec2-cr-linux-01 (9.60 MB, application/zip)
2012-03-26 01:50 PST, WebKit Review Bot
no flags Details
Patch (173.83 KB, patch)
2012-03-27 08:13 PST, Joe Thomas
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-07-10 14:47:24 PST
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 From 2009-07-11 19:20:07 PST -------
The Mozilla bug is:

https://bugzilla.mozilla.org/show_bug.cgi?id=503720
------- Comment #2 From 2011-08-21 14:35:34 PST -------
IE9 has these: http://msdn.microsoft.com/en-us/ie/ff468705.aspx#_CSS3_Values_Units
------- Comment #3 From 2011-10-03 11:30:42 PST -------
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 From 2012-01-05 11:35:44 PST -------
I am working on this.
------- Comment #5 From 2012-01-05 11:38:21 PST -------
Joe you're my hero.
------- Comment #6 From 2012-01-06 16:17:04 PST -------
Created an attachment (id=121518) [details]
Initial Version
------- Comment #7 From 2012-01-06 16:21:17 PST -------
Created an attachment (id=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 From 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 From 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 From 2012-01-09 14:00:16 PST -------
(From update of attachment 121518 [details])
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 From 2012-01-09 14:00:32 PST -------
Also, your ChangeLog shoudl explain your change in greater detail.
------- Comment #12 From 2012-01-11 16:39:19 PST -------
Created an attachment (id=122128) [details]
Patch2

Added new test case. Also more details to ChangeLog.
------- Comment #13 From 2012-01-23 14:04:07 PST -------
Can someone please review this?  Thanks.
------- Comment #14 From 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 From 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 From 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 From 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 From 2012-01-27 15:12:21 PST -------
(From update of attachment 122128 [details])
vw vh are relative units and it needs to be updated with window resize which is missing in the current patch.
------- Comment #19 From 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 From 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 From 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 From 2012-02-07 18:43:34 PST -------
Created an attachment (id=125981) [details]
Patch-with-windowResize-Support

This patch includes support for window-resizing(the unit value changes if the window gets resized.)
------- Comment #23 From 2012-02-07 20:46:14 PST -------
(From update of attachment 125981 [details])
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 From 2012-02-07 21:34:52 PST -------
(From update of attachment 125981 [details])
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 From 2012-02-07 22:49:41 PST -------
Created an attachment (id=126010) [details]
Patch-updated

Fixed the failing tests
------- Comment #26 From 2012-02-08 03:29:48 PST -------
(From update of attachment 126010 [details])
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 From 2012-02-08 03:36:52 PST -------
(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.
------- Comment #28 From 2012-02-08 15:08:42 PST -------
(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.

> > 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 From 2012-02-08 15:11:35 PST -------
(In reply to comment #27)
> (From update of attachment 126010 [details] [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 From 2012-02-09 00:48:20 PST -------
(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.

> > > 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 From 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] [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 From 2012-02-09 15:14:48 PST -------
Created an attachment (id=126385) [details]
Patch-updated-I

Review comments incorporated
------- Comment #33 From 2012-02-11 12:20:22 PST -------
(From update of attachment 126385 [details])
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 From 2012-02-11 12:33:46 PST -------
(From update of attachment 126385 [details])
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 From 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 From 2012-02-15 14:35:31 PST -------
Created an attachment (id=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 From 2012-02-15 17:00:46 PST -------
(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.

> 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 From 2012-02-15 20:24:38 PST -------
Looks good to me, please just address Luke's comments.
------- Comment #39 From 2012-02-15 22:40:26 PST -------
(In reply to comment #37)
> (From update of attachment 127238 [details] [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 From 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 From 2012-02-16 14:12:18 PST -------
Created an attachment (id=127439) [details]
Patch-02-16

Patch which uses viewport relative units more widely
------- Comment #42 From 2012-02-16 16:56:52 PST -------
The test coverage seems weak.
------- Comment #43 From 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 From 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 From 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 From 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 From 2012-02-21 17:22:44 PST -------
Created an attachment (id=128091) [details]
Patch-with-More-testCases

Patch with more test cases added.
------- Comment #48 From 2012-02-22 17:19:36 PST -------
Created an attachment (id=128343) [details]
patch-Updated-TestCases

Added more test cases.
------- Comment #49 From 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 From 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 From 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 From 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 From 2012-03-02 11:26:37 PST -------
(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.
------- Comment #54 From 2012-03-02 19:25:36 PST -------
Created an attachment (id=129988) [details]
Patch-incorporated-Hyatt's-Review-Comments

Incorporated Hyatt's Review comments
------- Comment #55 From 2012-03-02 19:26:56 PST -------
(In reply to comment #53)
> (From update of attachment 128343 [details] [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 From 2012-03-02 23:09:35 PST -------
r=me
------- Comment #57 From 2012-03-03 00:35:53 PST -------
(From update of attachment 129988 [details])
Clearing flags on attachment: 129988

Committed r109656: <http://trac.webkit.org/changeset/109656>
------- Comment #58 From 2012-03-03 00:36:03 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #59 From 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 From 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 From 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 From 2012-03-05 12:57:01 PST -------
This was rolled out in http://trac.webkit.org/changeset/109785
------- Comment #63 From 2012-03-10 09:31:01 PST -------
Created an attachment (id=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 From 2012-03-11 06:37:27 PST -------
(From update of attachment 131175 [details])
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 From 2012-03-11 06:38:01 PST -------
Created an attachment (id=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 From 2012-03-11 09:31:48 PST -------
Created an attachment (id=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 From 2012-03-11 11:27:31 PST -------
(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 From 2012-03-12 08:49:31 PST -------
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 From 2012-03-12 15:15:29 PST -------
(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 From 2012-03-25 23:35:19 PST -------
Created an attachment (id=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 From 2012-03-26 00:42:06 PST -------
(From update of attachment 133734 [details])
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 From 2012-03-26 00:42:17 PST -------
Created an attachment (id=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 From 2012-03-26 01:50:42 PST -------
(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
------- Comment #74 From 2012-03-26 01:50:52 PST -------
Created an attachment (id=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 From 2012-03-26 09:59:34 PST -------
(In reply to comment #73)
> (From update of attachment 133734 [details] [details])
> Attachment 133734 [details] [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 From 2012-03-26 17:17:05 PST -------
(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 From 2012-03-27 08:13:17 PST -------
Created an attachment (id=134068) [details]
Patch

Previously failed test case should fix by this patch.
------- Comment #78 From 2012-03-27 11:27:51 PST -------
(From update of attachment 134068 [details])
r=me
------- Comment #79 From 2012-03-27 11:28:33 PST -------
r=me
------- Comment #80 From 2012-03-27 12:20:23 PST -------
(From update of attachment 134068 [details])
Clearing flags on attachment: 134068

Committed r112301: <http://trac.webkit.org/changeset/112301>
------- Comment #81 From 2012-03-27 12:21:24 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #82 From 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