Use css-device-adapt constraining for legacy viewport tags.

Use css-device-adapt constraining for legacy viewport tags.

This CL is trying to take the part of Kenneth's CL [1] that is about using
the constraining procedure from the CSS Device Adaptation spec for legacy
meta tags, as well as for @viewport descriptors, to better match the
specification. Descriptors translated from meta tags are not cascaded with
@viewport rules but are only applied if @viewport from author style is not
present. See thread [2] on www-style.

ViewportArguments::resolve had different modes for resolving viewport
descriptors depending on the source. This CL translates legacy meta viewport
tags into descriptor values as specified by the css-device-adapt spec, but
they are not cascaded together as currently spec'ed. The prioritization is
still the same, overwriting descriptors from lower priority sources.

XHTML MP viewport is implemented through UA styles using @viewport.

[1] https://codereview.chromium.org/19555002/
[2] http://lists.w3.org/Archives/Public/www-style/2012May/0371.html

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

[kenneth.r.christiansen, 2013-09-02 09:10:48]

https://codereview.chromium.org/23742003/diff/1/Source/core/css/CSSDefaultSty...
File Source/core/css/CSSDefaultStyleSheets.h (right):

https://codereview.chromium.org/23742003/diff/1/Source/core/css/CSSDefaultSty...
Source/core/css/CSSDefaultStyleSheets.h:38: static RuleSet* defaultXHtmlMpStyle;
Maybe just write out MP

https://codereview.chromium.org/23742003/diff/1/Source/core/css/CSSParser-in.cpp
File Source/core/css/CSSParser-in.cpp (right):

https://codereview.chromium.org/23742003/diff/1/Source/core/css/CSSParser-in....
Source/core/css/CSSParser-in.cpp:1720: if
(!RuntimeEnabledFeatures::cssViewportEnabled() && m_context.mode != UASheetMode)
Add comment: // Allow @viewport rules from UA stylesheets even if the feature is
disabled

https://codereview.chromium.org/23742003/diff/1/Source/core/css/resolver/Scop...
File Source/core/css/resolver/ScopedStyleResolver.cpp (right):

https://codereview.chromium.org/23742003/diff/1/Source/core/css/resolver/Scop...
Source/core/css/resolver/ScopedStyleResolver.cpp:433:
resolver->collectViewportRules(m_authorStyle.get(), true);
true? We need an enum instead

https://codereview.chromium.org/23742003/diff/1/Source/core/css/resolver/Styl...
File Source/core/css/resolver/StyleResolver.h (right):

https://codereview.chromium.org/23742003/diff/1/Source/core/css/resolver/Styl...
Source/core/css/resolver/StyleResolver.h:270: void
collectViewportRules(RuleSet*, bool author);
We try hard to avoid the boolean trap

https://codereview.chromium.org/23742003/diff/1/Source/core/dom/Document.h
File Source/core/dom/Document.h (right):

https://codereview.chromium.org/23742003/diff/1/Source/core/dom/Document.h#ne...
Source/core/dom/Document.h:294: bool useLegacyViewportArguments();
Might need a comment explaining what that actually means

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

https://codereview.chromium.org/23742003/diff/1/Source/core/dom/ViewportArgum...
Source/core/dom/ViewportArguments.h:56: CSSDeviceAdaptation
Maybe rename to StyleSheet ?

[kenneth.r.christiansen, 2013-09-02 09:14:16]

https://codereview.chromium.org/23742003/diff/1/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/23742003/diff/1/Source/core/dom/Document.cpp#...
Source/core/dom/Document.cpp:2928: if (origin < m_legacyViewportArguments.type)
I guess this needs a comment

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

https://codereview.chromium.org/23742003/diff/1/Source/core/dom/ViewportArgum...
Source/core/dom/ViewportArguments.cpp:99: // 3. Resolve non-"auto" lengths to
pixel lengths.
So all this is the same as in my patch?

[rune, 2013-09-02 11:29:30]

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

https://codereview.chromium.org/23742003/diff/1/Source/core/dom/ViewportArgum...
Source/core/dom/ViewportArguments.cpp:99: // 3. Resolve non-"auto" lengths to
pixel lengths.
On 2013/09/02 09:14:16, kenneth.r.christiansen wrote:
> So all this is the same as in my patch?

I have not actually diffed - can do that. I did not remove the resolving of
'auto' for initial scale which made at least on test fail for you.

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

https://codereview.chromium.org/23742003/diff/1/Source/core/dom/ViewportArgum...
Source/core/dom/ViewportArguments.h:56: CSSDeviceAdaptation
On 2013/09/02 09:10:49, kenneth.r.christiansen wrote:
> Maybe rename to StyleSheet ?

Yes. Perhaps:

UserAgent => UserAgentStyleSheet (or UAStyleSheet)
CSSDeviceAdaptation => AuthorStyleSheet

[kenneth.r.christiansen, 2013-09-02 11:30:53]

> I have not actually diffed - can do that. I did not remove the resolving of
> 'auto' for initial scale which made at least on test fail for you.

I think it would be a good idea

> Yes. Perhaps:
> 
> UserAgent => UserAgentStyleSheet (or UAStyleSheet)
> CSSDeviceAdaptation => AuthorStyleSheet

Yeah that is better

[rune, 2013-09-02 11:53:39]

https://codereview.chromium.org/23742003/diff/1/Source/core/css/CSSDefaultSty...
File Source/core/css/CSSDefaultStyleSheets.h (right):

https://codereview.chromium.org/23742003/diff/1/Source/core/css/CSSDefaultSty...
Source/core/css/CSSDefaultStyleSheets.h:38: static RuleSet* defaultXHtmlMpStyle;
On 2013/09/02 09:10:49, kenneth.r.christiansen wrote:
> Maybe just write out MP

Done.

https://codereview.chromium.org/23742003/diff/1/Source/core/css/CSSParser-in.cpp
File Source/core/css/CSSParser-in.cpp (right):

https://codereview.chromium.org/23742003/diff/1/Source/core/css/CSSParser-in....
Source/core/css/CSSParser-in.cpp:1720: if
(!RuntimeEnabledFeatures::cssViewportEnabled() && m_context.mode != UASheetMode)
On 2013/09/02 09:10:49, kenneth.r.christiansen wrote:
> Add comment: // Allow @viewport rules from UA stylesheets even if the feature
is
> disabled

Done.

https://codereview.chromium.org/23742003/diff/1/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/23742003/diff/1/Source/core/dom/Document.cpp#...
Source/core/dom/Document.cpp:2928: if (origin < m_legacyViewportArguments.type)
On 2013/09/02 09:14:16, kenneth.r.christiansen wrote:
> I guess this needs a comment

Done.

https://codereview.chromium.org/23742003/diff/1/Source/core/dom/Document.h
File Source/core/dom/Document.h (right):

https://codereview.chromium.org/23742003/diff/1/Source/core/dom/Document.h#ne...
Source/core/dom/Document.h:294: bool useLegacyViewportArguments();
On 2013/09/02 09:10:49, kenneth.r.christiansen wrote:
> Might need a comment explaining what that actually means

Done.

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

https://codereview.chromium.org/23742003/diff/1/Source/core/dom/ViewportArgum...
Source/core/dom/ViewportArguments.h:56: CSSDeviceAdaptation
On 2013/09/02 11:29:30, rune wrote:
> On 2013/09/02 09:10:49, kenneth.r.christiansen wrote:
> > Maybe rename to StyleSheet ?
> 
> Yes. Perhaps:
> 
> UserAgent => UserAgentStyleSheet (or UAStyleSheet)
> CSSDeviceAdaptation => AuthorStyleSheet

Done.

https://codereview.chromium.org/23742003/diff/13001/Source/core/css/resolver/...
File Source/core/css/resolver/StyleResolver.h (left):

https://codereview.chromium.org/23742003/diff/13001/Source/core/css/resolver/...
Source/core/css/resolver/StyleResolver.h:269: // FIXME: Likely belongs on
viewportStyleResolver.
Feels even more like it now. Should I move it as part of this CL?

[kenneth.r.christiansen, 2013-09-02 11:55:28]

lgtm, but still a few comments

https://codereview.chromium.org/23742003/diff/13001/Source/core/css/CSSDefaul...
File Source/core/css/CSSDefaultStyleSheets.cpp (right):

https://codereview.chromium.org/23742003/diff/13001/Source/core/css/CSSDefaul...
Source/core/css/CSSDefaultStyleSheets.cpp:50: RuleSet*
CSSDefaultStyleSheets::defaultXHtmlMobileProfileStyle;
Same

https://codereview.chromium.org/23742003/diff/13001/Source/core/css/CSSDefaul...
File Source/core/css/CSSDefaultStyleSheets.h (right):

https://codereview.chromium.org/23742003/diff/13001/Source/core/css/CSSDefaul...
Source/core/css/CSSDefaultStyleSheets.h:38: static RuleSet*
defaultXHtmlMobileProfileStyle;
I guess this should be XHTML according to style

https://codereview.chromium.org/23742003/diff/13001/Source/core/css/resolver/...
File Source/core/css/resolver/StyleResolver.h (right):

https://codereview.chromium.org/23742003/diff/13001/Source/core/css/resolver/...
Source/core/css/resolver/StyleResolver.h:268: enum ViewportOrigin {
ViewportUserAgent, ViewportAuthor };
Wouldn't UserAgentOrigin, AuthorOrigin not make more sense?

like what is a viewport user agent?

https://codereview.chromium.org/23742003/diff/13001/Source/core/dom/Document.h
File Source/core/dom/Document.h (right):

https://codereview.chromium.org/23742003/diff/13001/Source/core/dom/Document....
Source/core/dom/Document.h:296: bool useLegacyViewportArguments();
We should really rename this ViewportArgument class at some point... 

useLegacyViewportHandling() ?

[rune, 2013-09-02 12:05:53]

https://codereview.chromium.org/23742003/diff/13001/Source/core/dom/Document.h
File Source/core/dom/Document.h (right):

https://codereview.chromium.org/23742003/diff/13001/Source/core/dom/Document....
Source/core/dom/Document.h:296: bool useLegacyViewportArguments();
On 2013/09/02 11:55:28, kenneth.r.christiansen wrote:
> We should really rename this ViewportArgument class at some point... 
> 
> useLegacyViewportHandling() ?

How about just useLegacyViewport() ?

[kenneth.r.christiansen, 2013-09-02 12:10:45]

> > We should really rename this ViewportArgument class at some point... 
> > 
> > useLegacyViewportHandling() ?
> 
> How about just useLegacyViewport() ?

It handles it when no author @viewport is set and a meta exists.

shouldUseLegacyViewportMeta() ? or s/Use/Consider?

[kenneth.r.christiansen, 2013-09-02 12:23:41]

https://codereview.chromium.org/23742003/diff/21001/Source/core/css/xhtmlmp.css
File Source/core/css/xhtmlmp.css (right):

https://codereview.chromium.org/23742003/diff/21001/Source/core/css/xhtmlmp.c...
Source/core/css/xhtmlmp.css:34: /* Ideally these should be removed. Currently
here to avoid test result regressions. */
You mean that these should be inherited from the UA sheet?

[rune, 2013-09-03 08:27:09]

https://codereview.chromium.org/23742003/diff/1/Source/core/css/resolver/Scop...
File Source/core/css/resolver/ScopedStyleResolver.cpp (right):

https://codereview.chromium.org/23742003/diff/1/Source/core/css/resolver/Scop...
Source/core/css/resolver/ScopedStyleResolver.cpp:433:
resolver->collectViewportRules(m_authorStyle.get(), true);
On 2013/09/02 09:10:49, kenneth.r.christiansen wrote:
> true? We need an enum instead

Done.

https://codereview.chromium.org/23742003/diff/1/Source/core/css/resolver/Styl...
File Source/core/css/resolver/StyleResolver.h (right):

https://codereview.chromium.org/23742003/diff/1/Source/core/css/resolver/Styl...
Source/core/css/resolver/StyleResolver.h:270: void
collectViewportRules(RuleSet*, bool author);
On 2013/09/02 09:10:49, kenneth.r.christiansen wrote:
> We try hard to avoid the boolean trap

Done.

https://codereview.chromium.org/23742003/diff/13001/Source/core/css/CSSDefaul...
File Source/core/css/CSSDefaultStyleSheets.cpp (right):

https://codereview.chromium.org/23742003/diff/13001/Source/core/css/CSSDefaul...
Source/core/css/CSSDefaultStyleSheets.cpp:50: RuleSet*
CSSDefaultStyleSheets::defaultXHtmlMobileProfileStyle;
On 2013/09/02 11:55:28, kenneth.r.christiansen wrote:
> Same

Done.

https://codereview.chromium.org/23742003/diff/13001/Source/core/css/CSSDefaul...
File Source/core/css/CSSDefaultStyleSheets.h (right):

https://codereview.chromium.org/23742003/diff/13001/Source/core/css/CSSDefaul...
Source/core/css/CSSDefaultStyleSheets.h:38: static RuleSet*
defaultXHtmlMobileProfileStyle;
On 2013/09/02 11:55:28, kenneth.r.christiansen wrote:
> I guess this should be XHTML according to style

Done.

https://codereview.chromium.org/23742003/diff/13001/Source/core/css/resolver/...
File Source/core/css/resolver/StyleResolver.h (right):

https://codereview.chromium.org/23742003/diff/13001/Source/core/css/resolver/...
Source/core/css/resolver/StyleResolver.h:268: enum ViewportOrigin {
ViewportUserAgent, ViewportAuthor };
On 2013/09/02 11:55:28, kenneth.r.christiansen wrote:
> Wouldn't UserAgentOrigin, AuthorOrigin not make more sense?
> 
> like what is a viewport user agent?

Done.

https://codereview.chromium.org/23742003/diff/13001/Source/core/dom/Document.h
File Source/core/dom/Document.h (right):

https://codereview.chromium.org/23742003/diff/13001/Source/core/dom/Document....
Source/core/dom/Document.h:296: bool useLegacyViewportArguments();
On 2013/09/02 12:05:53, rune wrote:
> On 2013/09/02 11:55:28, kenneth.r.christiansen wrote:
> > We should really rename this ViewportArgument class at some point... 
> > 
> > useLegacyViewportHandling() ?
> 
> How about just useLegacyViewport() ?

Method now removed.

[kenneth.r.christiansen, 2013-09-03 08:55:56]

https://codereview.chromium.org/23742003/diff/27001/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/23742003/diff/27001/Source/core/dom/Document....
Source/core/dom/Document.cpp:673: if
(m_docType->publicId().startsWith("-//wapforum//dtd xhtml mobile 1.", /*
caseSensitive */ false))
WebKit recently moved this to another method (patch by kling). You might want to
consider that

https://codereview.chromium.org/23742003/diff/27001/Source/core/dom/Document.h
File Source/core/dom/Document.h (right):

https://codereview.chromium.org/23742003/diff/27001/Source/core/dom/Document....
Source/core/dom/Document.h:297: bool hasLegacyViewportTag() const { return
m_legacyViewportArguments.type != ViewportArguments::UserAgentStyleSheet; }
What about the CSSDeviceAdaptation type (which I think we renamed)?

https://codereview.chromium.org/23742003/diff/27001/Source/core/dom/ViewportA...
File Source/core/dom/ViewportArguments.h (right):

https://codereview.chromium.org/23742003/diff/27001/Source/core/dom/ViewportA...
Source/core/dom/ViewportArguments.h:57: AuthorStyleSheet
This one

[rune, 2013-09-03 09:24:59]

https://codereview.chromium.org/23742003/diff/27001/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/23742003/diff/27001/Source/core/dom/Document....
Source/core/dom/Document.cpp:673: if
(m_docType->publicId().startsWith("-//wapforum//dtd xhtml mobile 1.", /*
caseSensitive */ false))
On 2013/09/03 08:55:57, kenneth.r.christiansen wrote:
> WebKit recently moved this to another method (patch by kling). You might want
to
> consider that

Could we do that in a separate patch? I am afraid to end up with an owner saying
it's too many changes again.

https://codereview.chromium.org/23742003/diff/27001/Source/core/dom/Document.h
File Source/core/dom/Document.h (right):

https://codereview.chromium.org/23742003/diff/27001/Source/core/dom/Document....
Source/core/dom/Document.h:297: bool hasLegacyViewportTag() const { return
m_legacyViewportArguments.type != ViewportArguments::UserAgentStyleSheet; }
On 2013/09/03 08:55:57, kenneth.r.christiansen wrote:
> What about the CSSDeviceAdaptation type (which I think we renamed)?

Done.

https://codereview.chromium.org/23742003/diff/27001/Source/core/dom/ViewportA...
File Source/core/dom/ViewportArguments.h (right):

https://codereview.chromium.org/23742003/diff/27001/Source/core/dom/ViewportA...
Source/core/dom/ViewportArguments.h:57: AuthorStyleSheet
On 2013/09/03 08:55:57, kenneth.r.christiansen wrote:
> This one

Done.

[kenneth.r.christiansen, 2013-09-03 10:23:20]

lgtm

[esprehn, 2013-09-04 11:16:37]

This seems to move a lot of stuff out from behind a flag, and adds support for
XHTML MP. Both of those need a launch email to blink-dev. Also you probably need
a bunch of layout tests. This is clearly changing behavior, but you haven't
added any.

I didn't think we had any plans to support any XHTML features going forward, do
people really use XHTML MP?

[kenneth.r.christiansen, 2013-09-04 11:23:54]

On 2013/09/04 11:16:37, esprehn wrote:
> This seems to move a lot of stuff out from behind a flag, and adds support for
> XHTML MP.

With the difference that this is not web facing. It allows user agents such as
Chrome or Opera to use @viewport in their UA sheets. I don't know whether we
need a launch email for that, but at least it shouldn't hurt.

> Both of those need a launch email to blink-dev. Also you probably need
> a bunch of layout tests. This is clearly changing behavior, but you haven't
> added any.

There are already tests for this, as @viewport is enabled for layout tests.

On the other hand it might be useful with tests showing that @viewport is not
enabled for web authors ie. web facing.

> I didn't think we had any plans to support any XHTML features going forward,
do
> people really use XHTML MP?

Browsers adding "Mobile" in their user agent usually get this. I remember
getting lots of this for the Nokia N9 browser.

Btw this doesn't add the support, XHTML MP is already supported, it just moves
the support to use a proper stylesheet instead of a viewport meta tag hack.

[esprehn, 2013-09-04 11:27:40]

On 2013/09/04 11:23:54, kenneth.r.christiansen wrote:
> On 2013/09/04 11:16:37, esprehn wrote:
> > This seems to move a lot of stuff out from behind a flag, and adds support
for
> > XHTML MP.
> 
> With the difference that this is not web facing. It allows user agents such as
> Chrome or Opera to use @viewport in their UA sheets. I don't know whether we
> need a launch email for that, but at least it shouldn't hurt.
> 
> > Both of those need a launch email to blink-dev. Also you probably need
> > a bunch of layout tests. This is clearly changing behavior, but you haven't
> > added any.
> 
> There are already tests for this, as @viewport is enabled for layout tests.
> 
> On the other hand it might be useful with tests showing that @viewport is not
> enabled for web authors ie. web facing.

So you're saying there's no added features in this patch? All behavior is the
same?

[esprehn, 2013-09-04 11:33:01]

LGTM for this assuming it doesn't add any new web exposed features and just
makes us use @viewport internally.

[kenneth.r.christiansen, 2013-09-04 11:39:01]

> So you're saying there's no added features in this patch? All behavior is the
same?

Yes, there are plenty of tests testing both @viewport (css3/device-adapt/) and
viewport meta (fast/viewport/) and the unit tests tests that the Android
behavior didn't change.

https://codereview.chromium.org/23742003/diff/36001/Source/core/css/CSSParser...
File Source/core/css/CSSParser-in.cpp (right):

https://codereview.chromium.org/23742003/diff/36001/Source/core/css/CSSParser...
Source/core/css/CSSParser-in.cpp:1721: if
(!RuntimeEnabledFeatures::cssViewportEnabled() && m_context.mode != UASheetMode)
This is a behavior change, but *only* for the UASheet, ie. not web facing

https://codereview.chromium.org/23742003/diff/36001/Source/core/css/CSSParser...
Source/core/css/CSSParser-in.cpp:11639: if
(!RuntimeEnabledFeatures::cssViewportEnabled() && m_context.mode != UASheetMode)
same

https://codereview.chromium.org/23742003/diff/36001/Source/core/css/CSSParser...
Source/core/css/CSSParser-in.cpp:11658:
ASSERT(RuntimeEnabledFeatures::cssViewportEnabled() || m_context.mode ==
UASheetMode);
same

https://codereview.chromium.org/23742003/diff/36001/Source/core/css/CSSParser...
Source/core/css/CSSParser-in.cpp:11718:
ASSERT(RuntimeEnabledFeatures::cssViewportEnabled() || m_context.mode ==
UASheetMode);
Same

https://codereview.chromium.org/23742003/diff/36001/Source/core/css/resolver/...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/23742003/diff/36001/Source/core/css/resolver/...
Source/core/css/resolver/StyleResolver.cpp:1020: if
(document().isMobileDocument())
This replaces the hack in Document.cpp

https://codereview.chromium.org/23742003/diff/36001/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/23742003/diff/36001/Source/core/dom/Document....
Source/core/dom/Document.cpp:674: m_isMobileDocument = true;
Hack using viewport meta, removed

[rune, 2013-09-04 14:21:30]

On 2013/09/04 11:33:01, esprehn wrote:
> LGTM for this assuming it doesn't add any new web exposed features and just
> makes us use @viewport internally.

Yes, that's the idea.

[rune, 2013-09-04 23:04:09]

Needs review by Source/web OWNER. @abarth?

[abarth-chromium, 2013-09-05 05:53:21]

Source/web LGTM

https://codereview.chromium.org/23742003/diff/46001/Source/web/tests/WebFrame...
File Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/23742003/diff/46001/Source/web/tests/WebFrame...
Source/web/tests/WebFrameTest.cpp:375: arguments.minWidth = arguments.maxWidth =
WebCore::Length(100, WebCore::Fixed);
Normally we would put each assignment on its own line.

[rune, 2013-09-05 07:05:52]

https://codereview.chromium.org/23742003/diff/46001/Source/web/tests/WebFrame...
File Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/23742003/diff/46001/Source/web/tests/WebFrame...
Source/web/tests/WebFrameTest.cpp:375: arguments.minWidth = arguments.maxWidth =
WebCore::Length(100, WebCore::Fixed);
On 2013/09/05 05:53:22, abarth wrote:
> Normally we would put each assignment on its own line.

Done.

[commit-bot: I haz the power, 2013-09-05 07:06:11]

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

[commit-bot: I haz the power, 2013-09-05 08:52:32]

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

[commit-bot: I haz the power, 2013-09-05 09:41:57]

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

[commit-bot: I haz the power, 2013-09-05 09:58:00]

Change committed as 157293