[Android WebView] Treat zero values in viewport arguments specially

[Android WebView] Treat zero values in viewport arguments specially

For compatibility with the old WebView, zero values in viewport
arguments must not be clamped. Zero values for "width" and "height"
must be interpreted as "device-width" and "device-height", respectively;
and zero "initial-scale" translates into 100%.

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

[mnaganov (inactive), 2013-08-30 09:36:06]

Alexandre, PTAL!

I think it's easier to merge the quirk from crbug.com/277369 and this one
together, as it would be tricky to allow the zero values quirk enabled without
enabling the layout expansion quirk. I haven't yet submitted a Chromium patch
that would use the setting, so renaming it now will not break anyone.

[aelias_OOO_until_Jul13, 2013-08-31 04:35:23]

Looks fine to me.

Kenneth, what do you think?  This is something else we need to cherry-pick into
M30 branch.  I think it shouldn't be too hard to reimplement this quirk as well
on top of CSS-adaptation?  I think we could land this first, then you could
delete and reimplement in terms of CSS-device-adaptation conversion in your
ongoing patch.

[rune, 2013-09-02 06:25:14]

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

https://codereview.chromium.org/23691017/diff/1/Source/core/dom/ViewportArgum...
Source/core/dom/ViewportArguments.h:57:
ViewportMetaLayoutSizeAndZeroValuesQuirk,
Is it important that these quirks can be turned on/off and change effect during
the lifetime of a WebView?

If not, I think I would have done the quirks during viewport meta tag
processing, and not as part of ViewportArguments::resolve. That will make
ViewportArguments::resolve less cluttery and make it possible to remove these
modes completely.

I see now that it's not possible to skip the extend-width part like that with
the current code, but still I'd like to know if changing that setting needs to
have an effect on a document that is already loaded in that WebView. Just to
know how we can rewrite this quirk for the new code.

https://codereview.chromium.org/23691017/diff/1/Source/web/WebSettingsImpl.h
File Source/web/WebSettingsImpl.h (right):

https://codereview.chromium.org/23691017/diff/1/Source/web/WebSettingsImpl.h#...
Source/web/WebSettingsImpl.h:200: // See http://crbug.com/277369 and
http://crbug.com/282130.
These bugs are not accessible to me. I guess that is because they're bugs in a
Google product not accessible to outsiders?

[kenneth.r.christiansen, 2013-09-02 06:45:03]

I am fine with this, so lgtm, but please clarify Rune's comments

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

https://codereview.chromium.org/23691017/diff/1/Source/core/dom/ViewportArgum...
Source/core/dom/ViewportArguments.h:57:
ViewportMetaLayoutSizeAndZeroValuesQuirk,
I would also really like to have these be runtime settings instead of tied into
the ViewportArguments as a specialized type

https://codereview.chromium.org/23691017/diff/1/Source/web/WebSettingsImpl.h
File Source/web/WebSettingsImpl.h (right):

https://codereview.chromium.org/23691017/diff/1/Source/web/WebSettingsImpl.h#...
Source/web/WebSettingsImpl.h:200: // See http://crbug.com/277369 and
http://crbug.com/282130.
Same to me... forbidden

[mnaganov (inactive), 2013-09-02 13:25:53]

Please consider the updated patch.

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

https://codereview.chromium.org/23691017/diff/1/Source/core/dom/ViewportArgum...
Source/core/dom/ViewportArguments.h:57:
ViewportMetaLayoutSizeAndZeroValuesQuirk,
On 2013/09/02 06:25:14, rune wrote:
> Is it important that these quirks can be turned on/off and change effect
during
> the lifetime of a WebView?
>

No. We can assume that they are constant during the lifetime of a WebView.

> If not, I think I would have done the quirks during viewport meta tag
> processing, and not as part of ViewportArguments::resolve. That will make
> ViewportArguments::resolve less cluttery and make it possible to remove these
> modes completely.
>

Not everything can be easily done during the processing stage, as sometimes
decisions depend on the values of other fields of the tag. While during the
processing stage the fields are considered independently. But thanks to your
proposal, I have found that at least it is possible to separate those two
quirks.
 
> I see now that it's not possible to skip the extend-width part like that with
> the current code, but still I'd like to know if changing that setting needs to
> have an effect on a document that is already loaded in that WebView. Just to
> know how we can rewrite this quirk for the new code.
>

https://codereview.chromium.org/23691017/diff/1/Source/web/WebSettingsImpl.h
File Source/web/WebSettingsImpl.h (right):

https://codereview.chromium.org/23691017/diff/1/Source/web/WebSettingsImpl.h#...
Source/web/WebSettingsImpl.h:200: // See http://crbug.com/277369 and
http://crbug.com/282130.
On 2013/09/02 06:25:14, rune wrote:
> These bugs are not accessible to me. I guess that is because they're bugs in a
> Google product not accessible to outsiders?

Sorry for that. Basically, http://crbug.com/277369 says that we wanted to pick
up https://bugs.webkit.org/show_bug.cgi?id=113363 and activate it conditionally.
And http://crbug.com/282130 says that we need to be able to process zero values
of the meta viewport tag according to the legacy code in
https://android.googlesource.com/platform/frameworks/base.git/+/android-4.2.2...
and
https://android.googlesource.com/platform/external/webkit/+/android-4.2.2_r1....,
also conditionally.

[kenneth.r.christiansen, 2013-09-02 13:44:00]

I think that I am fine with this. Let's see what Rune says

[rune, 2013-09-02 20:08:58]

On 2013/09/02 13:44:00, kenneth.r.christiansen wrote:
> I think that I am fine with this. Let's see what Rune says

Yes. The most important for me was to figure out the requirements if/when we
need to change this after @viewport changes.

I did have two comments, though.

[rune, 2013-09-02 20:09:05]

https://codereview.chromium.org/23691017/diff/7001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/23691017/diff/7001/Source/web/WebViewImpl.cpp...
Source/web/WebViewImpl.cpp:3020: if
(page()->settings().viewportMetaZeroValuesQuirk() && adjustedArguments.type ==
ViewportArguments::ViewportMeta) {
Looking at the if-test two lines further up, this if triggers if
viewportMetaLayoutSizeQuirk is off and viewportMetaZeroValuesQuirk is on. That
doesn't make sense if they could have been one quirk like in the previous patch
set?

https://codereview.chromium.org/23691017/diff/7001/Source/web/WebViewImpl.cpp...
Source/web/WebViewImpl.cpp:3022: adjustedArguments.zoom = 1.0;
If I understand this correctly, this could also have been done at parse-time
right after processArguments() in Document::processViewport().

[mnaganov (inactive), 2013-09-03 09:07:27]

https://codereview.chromium.org/23691017/diff/7001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/23691017/diff/7001/Source/web/WebViewImpl.cpp...
Source/web/WebViewImpl.cpp:3020: if
(page()->settings().viewportMetaZeroValuesQuirk() && adjustedArguments.type ==
ViewportArguments::ViewportMeta) {
On 2013/09/02 20:09:06, rune wrote:
> Looking at the if-test two lines further up, this if triggers if
> viewportMetaLayoutSizeQuirk is off and viewportMetaZeroValuesQuirk is on. That
> doesn't make sense if they could have been one quirk like in the previous
patch
> set?

I was proposing to merge those two quirks because I was only thinking in terms
of ViewportArguments::resolve implementation and didn't see other way around.
But as we have moved the processing of the "zero values" quirk into the parsing
stage, they actually can be independent. As I've understood from the discussions
with Adam Barth and Eric Seidel, they would prefer to deal with small
independent quirks than a big complex one.

https://codereview.chromium.org/23691017/diff/7001/Source/web/WebViewImpl.cpp...
Source/web/WebViewImpl.cpp:3022: adjustedArguments.zoom = 1.0;
On 2013/09/02 20:09:06, rune wrote:
> If I understand this correctly, this could also have been done at parse-time
> right after processArguments() in Document::processViewport().

Thanks for the suggestion! Moved.

[rune, 2013-09-03 09:12:57]

Rubber stamp

[mnaganov (inactive), 2013-09-03 09:15:37]

Adam, please take a look! Another WebView quirk.

[abarth-chromium, 2013-09-03 16:19:30]

lgtm

[commit-bot: I haz the power, 2013-09-03 16:19:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mnaganov@chromium.org/23691017/14001

[commit-bot: I haz the power, 2013-09-03 17:57:27]

Change committed as 157140