[css-device-adapt] Implemented spec changes.

[css-device-adapt] Implemented spec changes.

Modified implementation to match spec changes:

- Allow zero lengths for initial and actual viewport sizes.
- auto width and height lengths depending on aspect-ratio, never on zoom.

BUG=None

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

[abarth-chromium, 2013-06-12 23:22:09]

Looks reasonable to me, but I'm not an expert on this code.

[kenneth.r.christiansen, 2013-06-12 23:24:44]

Great stuff :-) lgtm as long as current viewport meta tag tests continue to
pass. Can you add try bots, apparently I don't have the rights.

[rune, 2013-06-12 23:29:48]

There is a regression in viewport-131.html. I need to look into that.

[rune, 2013-06-13 13:08:34]

On 2013/06/12 23:29:48, rune wrote:
> There is a regression in viewport-131.html. I need to look into that.

Fixed by Patch Set 2.

[kenneth.r.christiansen, 2013-06-13 13:46:41]

https://codereview.chromium.org/16826008/diff/8001/Source/core/dom/ViewportAr...
File Source/core/dom/ViewportArguments.cpp (right):

https://codereview.chromium.org/16826008/diff/8001/Source/core/dom/ViewportAr...
Source/core/dom/ViewportArguments.cpp:158: resultWidth = resultHeight *
initialViewportSize.width() / initialViewportSize.height();
I am a bit afraid that  this might break Android, as initialViewportSize depends
on orientation, where as deviceSize is always given portrait mode.

[rune, 2013-06-13 14:32:32]

On 2013/06/13 13:46:41, kenneth.r.christiansen wrote:
>
https://codereview.chromium.org/16826008/diff/8001/Source/core/dom/ViewportAr...
> File Source/core/dom/ViewportArguments.cpp (right):
> 
>
https://codereview.chromium.org/16826008/diff/8001/Source/core/dom/ViewportAr...
> Source/core/dom/ViewportArguments.cpp:158: resultWidth = resultHeight *
> initialViewportSize.width() / initialViewportSize.height();
> I am a bit afraid that  this might break Android, as initialViewportSize
depends
> on orientation, where as deviceSize is always given portrait mode.

Are you sure? My current Chrome on Nexus 4 reflows on rotation, resulting in
width based on orientation, for all three below:

<meta name="viewport" content="initial-scale=1">
<meta name="viewport" content="width=device-width, initial-scale=1">
<meta name="viewport" content="width=device-width">

Also, PageScaleConstraintsSet::updatePageDefinedConstraints passes viewSize for
both initialViewportSize and deviceSize to ViewportArguments::resolve.

[kenneth.r.christiansen, 2013-06-13 14:55:21]

Adding John and Peter to the discussion

[rune, 2013-06-26 09:13:38]

On 2013/06/13 14:55:21, kenneth.r.christiansen wrote:
> Adding John and Peter to the discussion

Stalled. You wanted John and Peter to comment on the
deviceSize/initialViewportSize orientation issue?

[Peter Beverloo, 2013-06-28 01:13:51]

Sorry for the delay, Rune!  John, could you please reply to the question Kenneth
left on PS2?

https://codereview.chromium.org/16826008/diff/8001/Source/core/dom/ViewportAr...
File Source/core/dom/ViewportArguments.cpp (right):

https://codereview.chromium.org/16826008/diff/8001/Source/core/dom/ViewportAr...
Source/core/dom/ViewportArguments.cpp:158: resultWidth = resultHeight *
initialViewportSize.width() / initialViewportSize.height();
Can you please add an ASSERT() to ensure that initialViewportSize.height() > 0? 
The current non-testing code path ensures this elsewhere, but if I have the
following line in a layout test it could cause a divide-by-zero:

internals.configurationForViewport(document, 1, 0, 0, 0, 0);

[rune, 2013-07-03 08:01:52]

https://codereview.chromium.org/16826008/diff/8001/Source/core/dom/ViewportAr...
File Source/core/dom/ViewportArguments.cpp (right):

https://codereview.chromium.org/16826008/diff/8001/Source/core/dom/ViewportAr...
Source/core/dom/ViewportArguments.cpp:158: resultWidth = resultHeight *
initialViewportSize.width() / initialViewportSize.height();
On 2013/06/28 01:13:51, Peter Beverloo wrote:
> Can you please add an ASSERT() to ensure that initialViewportSize.height() >
0?

Added.

> The current non-testing code path ensures this elsewhere, but if I have the
> following line in a layout test it could cause a divide-by-zero:
> 
> internals.configurationForViewport(document, 1, 0, 0, 0, 0);

It's ensured also for the configurationForViewport in the if-clause right above,
but it's not that obvious so an ASSERT is warranted.

[jochen (gone - plz use gerrit), 2013-07-09 12:36:16]

lgtm

[commit-bot: I haz the power, 2013-07-09 12:47:31]

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

[commit-bot: I haz the power, 2013-07-09 13:36:06]

Retried try job too often on linux_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...

[apavlov, 2013-07-10 08:15:05]

On 2013/07/09 13:36:06, I haz the power (commit-bot) wrote:
> Retried try job too often on linux_layout_rel for step(s) webkit_tests
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...

Viewport tests failed...

[rune, 2013-07-13 22:27:02]

On 2013/07/10 08:15:05, apavlov wrote:
> On 2013/07/09 13:36:06, I haz the power (commit-bot) wrote:
> > Retried try job too often on linux_layout_rel for step(s) webkit_tests
> >
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...
> 
> Viewport tests failed...

Corrected expectations (those tests were added to master after submission of
this patch) according to the changes in this patch that uses initial viewport
size instead of device size for aspect ratio when resolving auto width/height.

Last patch contains an unrelated change in ViewportStyleResolver.cpp due to a
new master base, sorry about that.

jochen lgtm'ed. Still awaiting approval from johnme, I think. I'm still on
vacation, not able to do any code changes or uploads the next two weeks.

[johnme, 2013-07-15 15:27:13]

lgtm

Sorry about the delay - these emails somehow missed my priority inbox.

Patch lgtm. Added to CQ.

I have a slight concern about the spec:

Suppose you have a partially-flexible website, whose layout breaks when narrower
than 640px. Suppose it also has a 100px tall position:fixed header.

A reasonable @viewport might be:
    @viewport {
        min-width: 640px;
        min-height: 196px; /* ensure 5 lines of text visible under header */
    }

On a 320x480 DIP device, the author probably expects the min-width to take
effect and the min-height to be completely ignored, giving a 640x960 px viewport
(i.e. the same as just @viewport { min-width: 640px; } would give).

But specifying a min-height, even if it's so small that it has no direct effect,
means height won't be auto in step 8, and so you miss out on the automatic
aspect ratio, and you actually end up with a 640x480 px viewport.

This seems unintuitive. But that needn't block this landing; we can tweak the
spec if you agree and update this later.

Rune wrote:
> Are you sure? My current Chrome on Nexus 4 reflows on rotation,
> resulting in width based on orientation, for all three below

Yup, unlike Mobile Safari, Chrome always uses the current orientation when
calculating device-width, so this should be fine.

[commit-bot: I haz the power, 2013-07-15 15:27:21]

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

[commit-bot: I haz the power, 2013-07-15 16:46:54]

Change committed as 154237