Reconstrain zoom value to the [min-zoom, max-zoom] range before it used

Reconstrain zoom value to the [min-zoom, max-zoom] range before it used

This bug is caused by we calculate zoom by ViewportSize but forgot reconstrain
to the [min-zoom, max-zoom] range. This patch add the reconstrain between
calculate zoom by ViewportSize and check user scalable.

BUG=656632
Committed: https://crrev.com/f36b4b14583e599557be01591205ec7219cd5c1a
Cr-Commit-Position: refs/heads/master@{#428888}

[chaopeng, 2016-10-28 23:44:59]

chaopeng@chromium.org changed reviewers:
+ bokan@chromium.org

[chaopeng, 2016-10-28 23:44:59]

PTAL. Thank you.

[bokan, 2016-10-31 13:58:19]

bokan@chromium.org changed reviewers:
+ rune@opera.com

[bokan, 2016-10-31 13:58:20]

This looks good to me, but the procedure was specifically defined in the
device-adaptation spec, though it seems to be slightly truncated/changed in the
latest iteration I could find
(https://www.w3.org/TR/css-device-adapt-1/#constraining-procedure).

Rune, does this look ok? Should this be defined in the spec as well?

https://codereview.chromium.org/2461143002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/tests/ViewportTest.cpp (right):

https://codereview.chromium.org/2461143002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/tests/ViewportTest.cpp:2926: TEST_F(ViewportTest,
viewportLimitsAdjustedForUserScale) {
Good catch!

[rune, 2016-10-31 14:45:04]

https://codereview.chromium.org/2461143002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/ViewportDescription.cpp (right):

https://codereview.chromium.org/2461143002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/ViewportDescription.cpp:204: // 9. Resolve
initial-scale value.
The numbering used to correspond with the numbering in the spec. This probably
broke when Florian rewrote the spec to use Bikeshed. There's no point in putting
9 here, at least.

This block is implementing
https://www.w3.org/TR/css-device-adapt-1/#handling-auto-zoom

That section already says the resulting viewport based zoom should be
constrained by the min/max values.

https://codereview.chromium.org/2461143002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/ViewportDescription.cpp:219: std::max);
This can be moved inside the block above, right?

[rune, 2016-10-31 14:45:58]

On 2016/10/31 13:58:20, bokan wrote:
> This looks good to me, but the procedure was specifically defined in the
> device-adaptation spec, though it seems to be slightly truncated/changed in
the
> latest iteration I could find
> (https://www.w3.org/TR/css-device-adapt-1/#constraining-procedure).
> 
> Rune, does this look ok? Should this be defined in the spec as well?

IIUC, it already is. See my comments inside the CL.

[bokan, 2016-10-31 14:47:55]

https://codereview.chromium.org/2461143002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/ViewportDescription.cpp (right):

https://codereview.chromium.org/2461143002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/ViewportDescription.cpp:204: // 9. Resolve
initial-scale value.
On 2016/10/31 14:45:03, rune wrote:
> The numbering used to correspond with the numbering in the spec. This probably
> broke when Florian rewrote the spec to use Bikeshed. There's no point in
putting
> 9 here, at least.
> 
> This block is implementing
> https://www.w3.org/TR/css-device-adapt-1/#handling-auto-zoom
> 
> That section already says the resulting viewport based zoom should be
> constrained by the min/max values.

In that case, since the numbers no longer correspond to anything, please remove
the numbers on all the comments.

[rune, 2016-10-31 15:04:17]

https://codereview.chromium.org/2461143002/diff/1/third_party/WebKit/Source/w...
File
third_party/WebKit/Source/web/tests/data/viewport/viewport-limits-adjusted-for-no-user-scale.html
(right):

https://codereview.chromium.org/2461143002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/tests/data/viewport/viewport-limits-adjusted-for-no-user-scale.html:1:
<head>
You should make this <!DOCTYPE html> and remove head and body tags while at it.

https://codereview.chromium.org/2461143002/diff/1/third_party/WebKit/Source/w...
File
third_party/WebKit/Source/web/tests/data/viewport/viewport-limits-adjusted-for-user-scale.html
(right):

https://codereview.chromium.org/2461143002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/tests/data/viewport/viewport-limits-adjusted-for-user-scale.html:1:
<head>
No need to use head or body tags, but you should use <!DOCTYPE html>

[chaopeng, 2016-10-31 16:56:40]

On 2016/10/31 15:04:17, rune wrote:
>
https://codereview.chromium.org/2461143002/diff/1/third_party/WebKit/Source/w...
> File
>
third_party/WebKit/Source/web/tests/data/viewport/viewport-limits-adjusted-for-no-user-scale.html
> (right):
> 
>
https://codereview.chromium.org/2461143002/diff/1/third_party/WebKit/Source/w...
>
third_party/WebKit/Source/web/tests/data/viewport/viewport-limits-adjusted-for-no-user-scale.html:1:
> <head>
> You should make this <!DOCTYPE html> and remove head and body tags while at
it.
> 
>
https://codereview.chromium.org/2461143002/diff/1/third_party/WebKit/Source/w...
> File
>
third_party/WebKit/Source/web/tests/data/viewport/viewport-limits-adjusted-for-user-scale.html
> (right):
> 
>
https://codereview.chromium.org/2461143002/diff/1/third_party/WebKit/Source/w...
>
third_party/WebKit/Source/web/tests/data/viewport/viewport-limits-adjusted-for-user-scale.html:1:
> <head>
> No need to use head or body tags, but you should use <!DOCTYPE html>

Updated. PTAL. Thank you.

[rune, 2016-10-31 22:24:24]

lgtm with nits

https://codereview.chromium.org/2461143002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/ViewportDescription.cpp (right):

https://codereview.chromium.org/2461143002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ViewportDescription.cpp:215: // below, if it
is ValueAuto here.
I didn't understand the second sentence: "It will reset below ...". Maybe just
drop it?

https://codereview.chromium.org/2461143002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/web/tests/data/viewport/viewport-limits-adjusted-for-no-user-scale.html
(right):

https://codereview.chromium.org/2461143002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/data/viewport/viewport-limits-adjusted-for-no-user-scale.html:2:

No empty line.

https://codereview.chromium.org/2461143002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/web/tests/data/viewport/viewport-limits-adjusted-for-user-scale.html
(right):

https://codereview.chromium.org/2461143002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/data/viewport/viewport-limits-adjusted-for-user-scale.html:2:

No need for an empty line.

[chaopeng, 2016-10-31 23:18:31]

The CQ bit was checked by chaopeng@chromium.org

[chaopeng, 2016-10-31 23:18:31]

The patchset sent to the CQ was uploaded after l-g-t-m from rune@opera.com
Link to the patchset: https://codereview.chromium.org/2461143002/#ps40001
(title: "for rune's nit")

[commit-bot: I haz the power, 2016-10-31 23:18:55]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-01 00:49:42]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-11-01 00:53:45]

Description was changed from

==========
Reconstrain zoom value to the [min-zoom, max-zoom] range before it used

This bug is caused by we calculate zoom by ViewportSize but forgot reconstrain
to the [min-zoom, max-zoom] range. This patch add the reconstrain between
calculate zoom by ViewportSize and check user scalable.

BUG=656632
==========

to

==========
Reconstrain zoom value to the [min-zoom, max-zoom] range before it used

This bug is caused by we calculate zoom by ViewportSize but forgot reconstrain
to the [min-zoom, max-zoom] range. This patch add the reconstrain between
calculate zoom by ViewportSize and check user scalable.

BUG=656632

Committed: https://crrev.com/f36b4b14583e599557be01591205ec7219cd5c1a
Cr-Commit-Position: refs/heads/master@{#428888}
==========

[commit-bot: I haz the power, 2016-11-01 00:53:46]

Patchset 3 (id:??) landed as
https://crrev.com/f36b4b14583e599557be01591205ec7219cd5c1a
Cr-Commit-Position: refs/heads/master@{#428888}