Recompute percentage based @viewport on resize.

Recompute percentage based @viewport on resize.

Percentage values were resolved to px lengths at cascade time for @viewport
widths and heights. This change keeps them as percentage values and recomputes
the actual viewport from the initial viewport and the percentage value when
the intial viewport changes (window resize). Likewise for viewport relative
units (vw, vh, vmin, vmax).

This fixes constrain-006 and constrain-007 in the w3c testsuite.

BUG=269132
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=157091

[kenneth.r.christiansen, 2013-08-07 08:32:35]

lgtm!

[kenneth.r.christiansen, 2013-08-07 08:33:00]

https://codereview.chromium.org/22549002/diff/1/Source/core/dom/ViewportArgum...
File Source/core/dom/ViewportArguments.h (right):

https://codereview.chromium.org/22549002/diff/1/Source/core/dom/ViewportArgum...
Source/core/dom/ViewportArguments.h:89: float width;
Guess it would be nice to get this be to a Length as well for consistency sake

[rune, 2013-08-07 09:07:43]

On 2013/08/07 08:33:00, kenneth.r.christiansen wrote:
>
https://codereview.chromium.org/22549002/diff/1/Source/core/dom/ViewportArgum...
> File Source/core/dom/ViewportArguments.h (right):
> 
>
https://codereview.chromium.org/22549002/diff/1/Source/core/dom/ViewportArgum...
> Source/core/dom/ViewportArguments.h:89: float width;
> Guess it would be nice to get this be to a Length as well for consistency sake

I considered that, but dropped it because I would have to introduce
device-width/device-height enums in Length. Both device-width/device-height as
lengths and the width/height member variables in ViewportArguments should be
gone RSN (as outlined in your CL).

[apavlov, 2013-08-07 09:25:54]

https://codereview.chromium.org/22549002/diff/1/Source/core/css/resolver/View...
File Source/core/css/resolver/ViewportStyleResolver.cpp (right):

https://codereview.chromium.org/22549002/diff/1/Source/core/css/resolver/View...
Source/core/css/resolver/ViewportStyleResolver.cpp:179: default:
Is there any particular idea behind putting "default:" in the middle, between
two cases?

https://codereview.chromium.org/22549002/diff/1/Source/core/dom/ViewportArgum...
File Source/core/dom/ViewportArguments.cpp (right):

https://codereview.chromium.org/22549002/diff/1/Source/core/dom/ViewportArgum...
Source/core/dom/ViewportArguments.cpp:71: static float
resolveViewportLength(const Length& length, const FloatSize&
initialViewportSize, bool horizontal)
optionally, you could have an enum instead of the "horizontal" boolean, as is
common with the core code...

[rune, 2013-08-07 10:06:15]

https://codereview.chromium.org/22549002/diff/1/Source/core/css/resolver/View...
File Source/core/css/resolver/ViewportStyleResolver.cpp (right):

https://codereview.chromium.org/22549002/diff/1/Source/core/css/resolver/View...
Source/core/css/resolver/ViewportStyleResolver.cpp:179: default:
On 2013/08/07 09:25:54, apavlov wrote:
> Is there any particular idea behind putting "default:" in the middle, between
> two cases?

Just to avoid an extra "return Length();" statement and have the assert for the
default case. There should have been a "// Fall through." comment there too.
Should I add an extra "return Length()" instead?

https://codereview.chromium.org/22549002/diff/1/Source/core/dom/ViewportArgum...
File Source/core/dom/ViewportArguments.cpp (right):

https://codereview.chromium.org/22549002/diff/1/Source/core/dom/ViewportArgum...
Source/core/dom/ViewportArguments.cpp:71: static float
resolveViewportLength(const Length& length, const FloatSize&
initialViewportSize, bool horizontal)
On 2013/08/07 09:25:54, apavlov wrote:
> optionally, you could have an enum instead of the "horizontal" boolean, as is
> common with the core code...

I'll fix.

[apavlov, 2013-08-07 10:12:10]

https://codereview.chromium.org/22549002/diff/1/Source/core/css/resolver/View...
File Source/core/css/resolver/ViewportStyleResolver.cpp (right):

https://codereview.chromium.org/22549002/diff/1/Source/core/css/resolver/View...
Source/core/css/resolver/ViewportStyleResolver.cpp:179: default:
On 2013/08/07 10:06:15, rune wrote:
> On 2013/08/07 09:25:54, apavlov wrote:
> > Is there any particular idea behind putting "default:" in the middle,
between
> > two cases?
> 
> Just to avoid an extra "return Length();" statement and have the assert for
the
> default case. There should have been a "// Fall through." comment there too.
> Should I add an extra "return Length()" instead?

Well, this doesn't look like code where two cases are [partially] semantically
equivalent and can be handled with a fallthrough. Instead, "return Length()" for
the default case is your best-effort approach to dealing with an error, right?
You might as well choose to return something different (a 0 length,
ExtendToZoom, or something else), so this should stand out as a separate return
statement to avoid readers' confusion.

[rune, 2013-08-07 11:30:04]

https://codereview.chromium.org/22549002/diff/1/Source/core/css/resolver/View...
File Source/core/css/resolver/ViewportStyleResolver.cpp (right):

https://codereview.chromium.org/22549002/diff/1/Source/core/css/resolver/View...
Source/core/css/resolver/ViewportStyleResolver.cpp:179: default:
On 2013/08/07 10:12:10, apavlov wrote:
> Well, this doesn't look like code where two cases are [partially] semantically
> equivalent and can be handled with a fallthrough. Instead, "return Length()"
for
> the default case is your best-effort approach to dealing with an error, right?
> You might as well choose to return something different (a 0 length,
> ExtendToZoom, or something else), so this should stand out as a separate
return
> statement to avoid readers' confusion.

Chose 0px in patch set 3.

https://codereview.chromium.org/22549002/diff/1/Source/core/dom/ViewportArgum...
File Source/core/dom/ViewportArguments.cpp (right):

https://codereview.chromium.org/22549002/diff/1/Source/core/dom/ViewportArgum...
Source/core/dom/ViewportArguments.cpp:71: static float
resolveViewportLength(const Length& length, const FloatSize&
initialViewportSize, bool horizontal)
On 2013/08/07 10:06:15, rune wrote:
> On 2013/08/07 09:25:54, apavlov wrote:
> > optionally, you could have an enum instead of the "horizontal" boolean, as
is
> > common with the core code...
> 
> I'll fix.

Done.

[kenneth.r.christiansen, 2013-09-02 15:28:40]

lgtm

[do-not-use, 2013-09-02 16:01:25]

lgtm

[darktears, 2013-09-02 16:31:47]

LGTM minus the nit.

https://codereview.chromium.org/22549002/diff/17001/Source/core/css/resolver/...
File Source/core/css/resolver/ViewportStyleResolver.cpp (right):

https://codereview.chromium.org/22549002/diff/17001/Source/core/css/resolver/...
Source/core/css/resolver/ViewportStyleResolver.cpp:163:
ASSERT(value->isPrimitiveValue());
This ASSERT is not needed. toCSSPrimitiveValue already check this condition.

[rune, 2013-09-02 19:25:44]

https://codereview.chromium.org/22549002/diff/17001/Source/core/css/resolver/...
File Source/core/css/resolver/ViewportStyleResolver.cpp (right):

https://codereview.chromium.org/22549002/diff/17001/Source/core/css/resolver/...
Source/core/css/resolver/ViewportStyleResolver.cpp:163:
ASSERT(value->isPrimitiveValue());
On 2013/09/02 16:31:47, darktears wrote:
> This ASSERT is not needed. toCSSPrimitiveValue already check this condition.

Done.

[commit-bot: I haz the power, 2013-09-02 19:40:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rune@opera.com/22549002/28001

[commit-bot: I haz the power, 2013-09-02 20:02:57]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac_layout.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-09-02 20:15:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rune@opera.com/22549002/45001

[commit-bot: I haz the power, 2013-09-02 21:31:26]

Change committed as 157091