Bug 85425 - Use suitable viewport values when a Mobile DTD is used.
: Use suitable viewport values when a Mobile DTD is used.
Status: RESOLVED FIXED
: WebKit
WebCore Misc.
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
: 86077
:
  Show dependency treegraph
 
Reported: 2012-05-02 15:20 PST by
Modified: 2012-05-17 07:55 PST (History)


Attachments
Patch (3.04 KB, patch)
2012-05-02 15:28 PST, Hugo Parente Lima
no flags Review Patch | Details | Formatted Diff | Diff
Patch (3.72 KB, patch)
2012-05-03 12:08 PST, Hugo Parente Lima
no flags Review Patch | Details | Formatted Diff | Diff
MiniBrowser --window-size 480x720 http://m.yahoo.com (without patch) (92.91 KB, image/png)
2012-05-04 10:46 PST, Hugo Parente Lima
no flags Details
MiniBrowser --window-size 480x720 http://m.yahoo.com (with patch) (125.83 KB, image/png)
2012-05-04 10:48 PST, Hugo Parente Lima
no flags Details
Patch (9.47 KB, patch)
2012-05-08 12:23 PST, Hugo Parente Lima
no flags Review Patch | Details | Formatted Diff | Diff
Patch (10.00 KB, patch)
2012-05-08 13:49 PST, Hugo Parente Lima
no flags Review Patch | Details | Formatted Diff | Diff
Patch (10.01 KB, patch)
2012-05-08 13:57 PST, Hugo Parente Lima
no flags Review Patch | Details | Formatted Diff | Diff
Patch (12.17 KB, patch)
2012-05-08 16:52 PST, Hugo Parente Lima
no flags Review Patch | Details | Formatted Diff | Diff
Patch (13.72 KB, patch)
2012-05-09 07:08 PST, Hugo Parente Lima
no flags Review Patch | Details | Formatted Diff | Diff
Patch (15.97 KB, patch)
2012-05-09 10:27 PST, Hugo Parente Lima
no flags Review Patch | Details | Formatted Diff | Diff
Patch (14.72 KB, patch)
2012-05-09 12:05 PST, Hugo Parente Lima
no flags Review Patch | Details | Formatted Diff | Diff
Patch (14.08 KB, patch)
2012-05-09 12:59 PST, Hugo Parente Lima
no flags Review Patch | Details | Formatted Diff | Diff
Patch (14.04 KB, patch)
2012-05-09 14:40 PST, Hugo Parente Lima
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-05-02 15:20:40 PST
WebKit currently ignores web sites with Mobile DTD's, as result mobile websites are rendered in a large canvas (980px IIRC) being unreadable without zooming on mobile devices.
------- Comment #1 From 2012-05-02 15:28:52 PST -------
Created an attachment (id=139892) [details]
Patch
------- Comment #2 From 2012-05-02 17:33:55 PST -------
Is this specified in HTML5?
------- Comment #3 From 2012-05-02 18:18:45 PST -------
No, it's just a doctype

http://en.wikipedia.org/wiki/XHTML_Mobile_Profile
------- Comment #4 From 2012-05-02 23:22:56 PST -------
(From update of attachment 139892 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=139892&action=review

> Source/WebCore/html/parser/HTMLConstructionSite.cpp:245
> +        m_document->useMobileViewportValues();

If you are trying to fake <meta name = "viewport" content = "width=device-width, user-scalable=no, target-densityDpi=device-dpi">, 
I'd just do m_document->processViewport("width=device-width, user-scalable=no, target-densityDpi=device-dpi") and then it'll go through the normal viewport processing logic.
------- Comment #5 From 2012-05-02 23:25:33 PST -------
and add a test case with mobile dtd to /LayoutTests/fast/viewport
------- Comment #6 From 2012-05-02 23:49:42 PST -------
> No, it's just a doctype

I don't understand this comment. How does this being a doctype make standardization unnecessary?
------- Comment #7 From 2012-05-03 01:43:14 PST -------
(In reply to comment #2)
> Is this specified in HTML5?

XHTML-MP is supposed to use device dimensions for layout which doesn't happen with fixed layout due to 980 (or similar values) being used as the default layout viewport on mobile devices.

Injecting a viewport meta tag (which is done on the N9, Android and possible other platforms) solves this nicely and makes most XHTML-MP pages render properly even though we do not support WAP extensions and have no intention to.
------- Comment #8 From 2012-05-03 01:44:50 PST -------
(From update of attachment 139892 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=139892&action=review

> Source/WebCore/ChangeLog:8
> +        Set the viewport width to device-width when the document uses a mobile DTD.

What is the definition of that? XHTML-MP?

> Source/WebCore/html/parser/HTMLConstructionSite.cpp:244
> +    if (doctype->publicId().contains("Mobile"))

I would be a lot more specific here. Did you look at how we did this for the N9? It doesn't seem so given Zalan's comments.
------- Comment #9 From 2012-05-03 08:57:24 PST -------
Do you have any specific examples of pages that don't look or behave nicely because of this?
------- Comment #10 From 2012-05-03 10:11:31 PST -------
(In reply to comment #9)
> Do you have any specific examples of pages that don't look or behave nicely because of this?

http://www.google.com :-)

If you open it with e.g. N9 user agent.
------- Comment #11 From 2012-05-03 10:15:31 PST -------
(In reply to comment #8)
> (From update of attachment 139892 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=139892&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        Set the viewport width to device-width when the document uses a mobile DTD.
> 
> What is the definition of that? XHTML-MP?

Yes it's.

> > Source/WebCore/html/parser/HTMLConstructionSite.cpp:244
> > +    if (doctype->publicId().contains("Mobile"))
> 
> I would be a lot more specific here. Did you look at how we did this for the N9? It doesn't seem so given Zalan's comments.

N9 does the magic on webkit side, not on the browser code and I don't have access to the webkit port used by grob :-/, btw I don't have access to projects.maemo anymore.
------- Comment #12 From 2012-05-03 10:19:06 PST -------
(In reply to comment #4)
> (From update of attachment 139892 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=139892&action=review
> 
> > Source/WebCore/html/parser/HTMLConstructionSite.cpp:245
> > +        m_document->useMobileViewportValues();
> 
> If you are trying to fake <meta name = "viewport" content = "width=device-width, user-scalable=no, target-densityDpi=device-dpi">, 
> I'd just do m_document->processViewport("width=device-width, user-scalable=no, target-densityDpi=device-dpi") and then it'll go through the normal viewport processing logic.

Seems feasible not changing the WebCore::Document API despite of being a bit slower, but I doubt the difference would be notifiable.
------- Comment #13 From 2012-05-03 10:23:21 PST -------
(In reply to comment #8)
> (From update of attachment 139892 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=139892&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        Set the viewport width to device-width when the document uses a mobile DTD.
> 
> What is the definition of that? XHTML-MP?
> 
> > Source/WebCore/html/parser/HTMLConstructionSite.cpp:244
> > +    if (doctype->publicId().contains("Mobile"))
> 
> I would be a lot more specific here. Did you look at how we did this for the N9? It doesn't seem so given Zalan's comments.

I agree this check is too weak, but I can do the check against all possibilities:

"-//WAPFORUM//DTD XHTML Mobile 1.0//EN"
"-//WAPFORUM//DTD XHTML Mobile 1.1//EN"
"-//WAPFORUM//DTD XHTML Mobile 1.2//EN"

What do you think?
------- Comment #14 From 2012-05-03 10:24:22 PST -------
(In reply to comment #5)
> and add a test case with mobile dtd to /LayoutTests/fast/viewport

Going to do that, thanks.
------- Comment #15 From 2012-05-03 11:15:39 PST -------
(In reply to comment #11)
> (In reply to comment #8)
> > (From update of attachment 139892 [details] [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=139892&action=review
> > 
> > > Source/WebCore/ChangeLog:8
> > > +        Set the viewport width to device-width when the document uses a mobile DTD.
> > 
> > What is the definition of that? XHTML-MP?
> 
> Yes it's.
> 
> > > Source/WebCore/html/parser/HTMLConstructionSite.cpp:244
> > > +    if (doctype->publicId().contains("Mobile"))
> > 
> > I would be a lot more specific here. Did you look at how we did this for the N9? It doesn't seem so given Zalan's comments.
> 
> N9 does the magic on webkit side, not on the browser code and I don't have access to the webkit port used by grob :-/, btw I don't have access to projects.maemo anymore.

n9's webkit branch is on gitorious.
https://gitorious.org/+qtwebkit-webkit2-dev/webkit/qtwebkit-webkit2-dev/blobs/master/WebCore/dom/Document.cpp
------- Comment #16 From 2012-05-03 11:18:49 PST -------
(In reply to comment #15)
> (In reply to comment #11)
> > (In reply to comment #8)
> > > (From update of attachment 139892 [details] [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=139892&action=review
> > > 
> > > > Source/WebCore/ChangeLog:8
> > > > +        Set the viewport width to device-width when the document uses a mobile DTD.
> > > 
> > > What is the definition of that? XHTML-MP?
> > 
> > Yes it's.
> > 
> > > > Source/WebCore/html/parser/HTMLConstructionSite.cpp:244
> > > > +    if (doctype->publicId().contains("Mobile"))
> > > 
> > > I would be a lot more specific here. Did you look at how we did this for the N9? It doesn't seem so given Zalan's comments.
> > 
> > N9 does the magic on webkit side, not on the browser code and I don't have access to the webkit port used by grob :-/, btw I don't have access to projects.maemo anymore.
> 
> n9's webkit branch is on gitorious.
> https://gitorious.org/+qtwebkit-webkit2-dev/webkit/qtwebkit-webkit2-dev/blobs/master/WebCore/dom/Document.cpp

Thanks very much! I didn't know that the code was on gitorious, so the patch will be a matter of port the n9 code plus write some test, thanks!
------- Comment #17 From 2012-05-03 11:27:47 PST -------
OK. If it's only google.com, then this should be an evangelism bug.

This is not a small deal. Entering magic modes based on unsupported DTDs is not something we should do unless there is huge and universally agreed benefit to it (in which case it should be standardized in HTML5).
------- Comment #18 From 2012-05-03 11:39:48 PST -------
(In reply to comment #17)
> OK. If it's only google.com, then this should be an evangelism bug.
> 
> This is not a small deal. Entering magic modes based on unsupported DTDs is not something we should do unless there is huge and universally agreed benefit to it (in which case it should be standardized in HTML5).

http://m.facebook.com also uses this DTD, i.e. not only google, but a lot of mobile versions of well known websites.

The default 980px canvas IMO already is a magic mode, we are just extending this magic mode when some DTD is found =], btw IMO it's not even a "mode" because we don't change anything in the way HTML or CSS is interpreted, just the canvas width and this can be overwritten by the viewport meta tag.
------- Comment #19 From 2012-05-03 12:04:56 PST -------
(In reply to comment #17)
> OK. If it's only google.com, then this should be an evangelism bug.
> 
> This is not a small deal. Entering magic modes based on unsupported DTDs is not something we should do unless there is huge and universally agreed benefit to it (in which case it should be standardized in HTML5).

While I totally agree with the statement of 'entering magic modes based on unsupported DTDs is not something we should do...', I am wondering if the fact, that it makes trunk compatible with iOS's Safari in this context (as far as the viewport width is concerned, not sure about the dpi) is a good enough reason to have it. 

iOS Safari uses different viewport canvas width depending whether the doctype instruction exists or not for the following simple test case
<!DOCTYPE html PUBLIC "-//WAPFORUM//DTD XHTML Mobile 1.0//EN" "http://www.wapforum.org/DTD/xhtml-mobile10.dtd">
<html><body>Hello world. Welcome to our XHTML MP tutorial.</body></html>
------- Comment #20 From 2012-05-03 12:08:14 PST -------
Created an attachment (id=140061) [details]
Patch
------- Comment #21 From 2012-05-03 13:08:11 PST -------
I think that if you want to go ahead with this, a webkit-dev discussion is in order. A lot of people who may want to weigh in are not on CC list of this bug.
------- Comment #22 From 2012-05-03 13:11:57 PST -------
(In reply to comment #21)
> I think that if you want to go ahead with this, a webkit-dev discussion is in order. A lot of people who may want to weigh in are not on CC list of this bug.

Sorry, I don't know who to add to the CC list, could you add to the CC list people you think should be CC'ed? or may be better to just post on webkit-dev about this bug?
------- Comment #23 From 2012-05-03 13:16:16 PST -------
btw, also add http://m.cnn.com and http://m.yahoo.com on the list of sites that doesn't look well on a 980px canvas zoomed out to fit in a 480px wide screen.
------- Comment #24 From 2012-05-03 13:17:24 PST -------
Posting on webkit-dev was what I suggested, yes.
------- Comment #25 From 2012-05-03 13:18:47 PST -------
(In reply to comment #23)
> btw, also add http://m.cnn.com and http://m.yahoo.com on the list of sites that doesn't look well on a 980px canvas zoomed out to fit in a 480px wide screen.

Oops, m.cnn.com also uses the viewport metatag, so it looks nicely.
------- Comment #26 From 2012-05-04 05:58:31 PST -------
(In reply to comment #11)
> (In reply to comment #8)
> > (From update of attachment 139892 [details] [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=139892&action=review
> > 
> > > Source/WebCore/ChangeLog:8
> > > +        Set the viewport width to device-width when the document uses a mobile DTD.
> > 
> > What is the definition of that? XHTML-MP?
> 
> Yes it's.
> 
> > > Source/WebCore/html/parser/HTMLConstructionSite.cpp:244
> > > +    if (doctype->publicId().contains("Mobile"))
> > 
> > I would be a lot more specific here. Did you look at how we did this for the N9? It doesn't seem so given Zalan's comments.
> 
> N9 does the magic on webkit side, not on the browser code and I don't have access to the webkit port used by grob :-/, btw I don't have access to projects.maemo anymore.

It is on gitorious :-)
------- Comment #27 From 2012-05-04 07:42:28 PST -------
(From update of attachment 140061 [details])
Could we get it as an opt-in/out thing? Maybe in settings or a USE macro.

Also could you show some screenshots of how it would look before and after for some websites?
------- Comment #28 From 2012-05-04 10:46:42 PST -------
Created an attachment (id=140272) [details]
MiniBrowser --window-size 480x720 http://m.yahoo.com (without patch)
------- Comment #29 From 2012-05-04 10:48:24 PST -------
Created an attachment (id=140273) [details]
MiniBrowser --window-size 480x720 http://m.yahoo.com (with patch)
------- Comment #30 From 2012-05-04 11:02:06 PST -------
(In reply to comment #27)
> (From update of attachment 140061 [details] [details])
> Could we get it as an opt-in/out thing? Maybe in settings or a USE macro.

An USE macro sounds better to me, I don't think someone will want to turn on/off this thing at runtime.

> Also could you show some screenshots of how it would look before and after for some websites?

Screenshots there =]
------- Comment #31 From 2012-05-04 12:19:03 PST -------
(In reply to comment #27)
> (From update of attachment 140061 [details] [details])
> Could we get it as an opt-in/out thing? Maybe in settings or a USE macro.
Wondering why would anyone opt out and render the content, which was originally targeting small screen devices on a 980px wide canvas?
(Not saying there shouldn't be a way to opt out, just curious)
------- Comment #32 From 2012-05-04 19:47:59 PST -------
(From update of attachment 140061 [details])
r- because we don't want this feature on desktop browsers. Please add a feature flag.
------- Comment #33 From 2012-05-05 05:01:54 PST -------
(In reply to comment #32)
> (From update of attachment 140061 [details] [details])
> r- because we don't want this feature on desktop browsers. Please add a feature flag.

Also Hugo, we would like a layout test both when using fixed layout and when not to ensure that this would never break desktop browsers anyway (we only use one feature set for Qt)

Also, please get back to me on Monday so that I can write an email to www-style regarding the CSS Device Adaptation spec.
------- Comment #34 From 2012-05-05 05:03:24 PST -------
(In reply to comment #19)
> (In reply to comment #17)
> > OK. If it's only google.com, then this should be an evangelism bug.
> > 
> > This is not a small deal. Entering magic modes based on unsupported DTDs is not something we should do unless there is huge and universally agreed benefit to it (in which case it should be standardized in HTML5).
> 
> While I totally agree with the statement of 'entering magic modes based on unsupported DTDs is not something we should do...', I am wondering if the fact, that it makes trunk compatible with iOS's Safari in this context (as far as the viewport width is concerned, not sure about the dpi) is a good enough reason to have it. 
> 
> iOS Safari uses different viewport canvas width depending whether the doctype instruction exists or not for the following simple test case
> <!DOCTYPE html PUBLIC "-//WAPFORUM//DTD XHTML Mobile 1.0//EN" "http://www.wapforum.org/DTD/xhtml-mobile10.dtd">
> <html><body>Hello world. Welcome to our XHTML MP tutorial.</body></html>

Zalan would it be possible for you to tests these things well on both iOS and Android and then create some tests? If you can find links to their code doing this that would be even better.
------- Comment #35 From 2012-05-08 06:38:52 PST -------
This is pretty much already specified in CSS Device Adaptation (http://www.w3.org/TR/css-device-adapt/)

Quote: "Certain DOCTYPEs (for instance XHTML Mobile Profile) are used to recognize mobile documents which are assumed to be designed for handheld devices, hence using the viewport size as the initial containing block size."
------- Comment #36 From 2012-05-08 06:48:21 PST -------
(In reply to comment #35)
> This is pretty much already specified in CSS Device Adaptation (http://www.w3.org/TR/css-device-adapt/)
> 
> Quote: "Certain DOCTYPEs (for instance XHTML Mobile Profile) are used to recognize mobile documents which are assumed to be designed for handheld devices, hence using the viewport size as the initial containing block size."

So, a feature flag still needed? even it being part of the (future) spec?
------- Comment #37 From 2012-05-08 10:40:50 PST -------
For the sake of archaeology, a link to the mailing list thread about the subject:

https://lists.webkit.org/pipermail/webkit-dev/2012-May/020558.html
------- Comment #38 From 2012-05-08 12:23:01 PST -------
Created an attachment (id=140765) [details]
Patch
------- Comment #39 From 2012-05-08 12:23:41 PST -------
Patch re-uploaded, but few notes:

- I don't know very much the webkit buildsystem(s), so the patch may have some mistakes, please correct me!
- Hope the new test is disabled on all ports, but Qt.

Kenneth, the patch uses the same code patch of a page using the viewport meta tag, so use or not use the info is up to the port, I may be wrong but I think this is the same reason why there's no test about using a fixed layout on fast/viewport/*.
------- Comment #40 From 2012-05-08 13:02:15 PST -------
(From update of attachment 140765 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=140765&action=review

> Source/WebCore/dom/Document.cpp:776
> +#if USE(VIEWPORT_ON_HTMLMP)

I think we rather want a USE(LEGACY_VIEWPORT_ADAPTION) or so, but you should really look at https://bugs.webkit.org/show_bug.cgi?id=55874 so that we handle the priority of the different viewport adaptions correctly and compatible with the N9, Android etc.

> Source/cmake/WebKitFeatures.cmake:97
> +    WEBKIT_OPTION_DEFINE(USE_VIEWPORT_ON_HTMLMP "Toogle special viewport size for HTML-MP pages" OFF)

Toogle legacy viewport adaptation
------- Comment #41 From 2012-05-08 13:49:19 PST -------
Created an attachment (id=140772) [details]
Patch
------- Comment #42 From 2012-05-08 13:57:43 PST -------
Created an attachment (id=140775) [details]
Patch
------- Comment #43 From 2012-05-08 14:00:16 PST -------
(From update of attachment 140772 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=140772&action=review

> Source/WebCore/ChangeLog:8
> +        Test: fast/viewport/mobile-dtd.html

It needs a test with both fixed layout and without and a test where the XHTML-MP has a viewport meta tag
------- Comment #44 From 2012-05-08 16:52:20 PST -------
Created an attachment (id=140817) [details]
Patch
------- Comment #45 From 2012-05-08 17:18:13 PST -------
(From update of attachment 140817 [details])
Attachment 140817 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12643836
------- Comment #46 From 2012-05-08 17:33:22 PST -------
(From update of attachment 140817 [details])
Attachment 140817 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12649548
------- Comment #47 From 2012-05-09 02:01:04 PST -------
(From update of attachment 140817 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=140817&action=review

> Source/WebCore/dom/Document.cpp:779
> +            && m_docType->publicId().startsWith("-//wapforum//dtd xhtml mobile 1.", false)) {

What does the false specify? /* caseSensitive */ false ?

> LayoutTests/fast/viewport/viewport-legacy-xhtmlmp-ordering.html:4
> +    <title>Default viewport value changed by a mobile doc type.</title>

It would be nice to write in the tests what is supposed to happen, or link to a spec.
------- Comment #48 From 2012-05-09 02:11:59 PST -------
(From update of attachment 140817 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=140817&action=review

> Source/WebCore/dom/Document.cpp:777
> +        if (m_viewportArguments.type < ViewportArguments::XHTMLMobileProfile

Looking at the html5 spec, I don't think <meta> can be followed by the doctype declaration, so not sure how can the current viewportargument type be greater than Implicit. Did I miss something here?
------- Comment #49 From 2012-05-09 06:39:38 PST -------
(In reply to comment #48)
> (From update of attachment 140817 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140817&action=review
> 
> > Source/WebCore/dom/Document.cpp:777
> > +        if (m_viewportArguments.type < ViewportArguments::XHTMLMobileProfile
> 
> Looking at the html5 spec, I don't think <meta> can be followed by the doctype declaration, so not sure how can the current viewportargument type be greater than Implicit. Did I miss something here?

There are many sites[1] that use the XHTML-MP doc type but also the viewport meta tag, so the non-legacy way of set the viewport takes precedence over the legacy ways.

[1] e.g.: http://m.cnn.com, http://m.facebook.com
------- Comment #50 From 2012-05-09 07:08:40 PST -------
Created an attachment (id=140938) [details]
Patch
------- Comment #51 From 2012-05-09 07:14:02 PST -------
(In reply to comment #49)
> (In reply to comment #48)
> > (From update of attachment 140817 [details] [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=140817&action=review
> > 
> > > Source/WebCore/dom/Document.cpp:777
> > > +        if (m_viewportArguments.type < ViewportArguments::XHTMLMobileProfile
> > 
> > Looking at the html5 spec, I don't think <meta> can be followed by the doctype declaration, so not sure how can the current viewportargument type be greater than Implicit. Did I miss something here?
> 
> There are many sites[1] that use the XHTML-MP doc type but also the viewport meta tag, so the non-legacy way of set the viewport takes precedence over the legacy ways.
> 
> [1] e.g.: http://m.cnn.com, http://m.facebook.com
Yes and that's indeed the right priority order.
What I meant was this: http://trac.webkit.org/browser/trunk/Source/WebCore/html/parser/HTMLTreeBuilder.cpp#L518

DocType doesn't get inserted, unless the HTMLBuilder is in InitialMode, which does not look to be possible if <meta> is reached already.
It's just nitpicking, if no one has objection, I don't mind having the check like that.
------- Comment #52 From 2012-05-09 07:28:18 PST -------
(In reply to comment #51)
> (In reply to comment #49)
> > (In reply to comment #48)
> > > (From update of attachment 140817 [details] [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=140817&action=review
> > > 
> > > > Source/WebCore/dom/Document.cpp:777
> > > > +        if (m_viewportArguments.type < ViewportArguments::XHTMLMobileProfile
> > > 
> > > Looking at the html5 spec, I don't think <meta> can be followed by the doctype declaration, so not sure how can the current viewportargument type be greater than Implicit. Did I miss something here?
> > 
> > There are many sites[1] that use the XHTML-MP doc type but also the viewport meta tag, so the non-legacy way of set the viewport takes precedence over the legacy ways.
> > 
> > [1] e.g.: http://m.cnn.com, http://m.facebook.com
> Yes and that's indeed the right priority order.
> What I meant was this: http://trac.webkit.org/browser/trunk/Source/WebCore/html/parser/HTMLTreeBuilder.cpp#L518
> 
> DocType doesn't get inserted, unless the HTMLBuilder is in InitialMode, which does not look to be possible if <meta> is reached already.
> It's just nitpicking, if no one has objection, I don't mind having the check like that.

Ah, got it, this can be turned into a assertion for the sake of sanity check or just stripped out.
------- Comment #53 From 2012-05-09 09:03:08 PST -------
> It's just nitpicking, if no one has objection, I don't mind having the check like that.

I think that having such unnecessary code would be objectionable, because it can confuse readers, and thus decrease hackability of the project. Good catch!

> Ah, got it, this can be turned into a assertion for the sake of sanity check or just stripped out.

Either seems fine to me. Please consider adding a regression test with a misplaced DOCTYPE.
------- Comment #54 From 2012-05-09 10:27:49 PST -------
Created an attachment (id=140971) [details]
Patch
------- Comment #55 From 2012-05-09 10:29:09 PST -------
(In reply to comment #53)
> > It's just nitpicking, if no one has objection, I don't mind having the check like that.
> 
> I think that having such unnecessary code would be objectionable, because it can confuse readers, and thus decrease hackability of the project. Good catch!
> 
> > Ah, got it, this can be turned into a assertion for the sake of sanity check or just stripped out.
> 
> Either seems fine to me. Please consider adding a regression test with a misplaced DOCTYPE.

For sure, check removed, test added.
------- Comment #56 From 2012-05-09 11:26:23 PST -------
(From update of attachment 140971 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=140971&action=review

> Source/WebCore/dom/Document.cpp:777
> +        if (!ownerElement()

Aren't this and the mainframe() check at updateViewportArguments() supposed to serve the same purpose? (that we only do meta viewport for mainframes) While they are probably in sync functionality wise at this point(!ownerelement() vs. page()->mainFrame() == frame()), it is somewhat error prone setup and can result in a situation, where the m_viewportArguments value is changed, but it's not dispatched further and the actual viewport ends being up out of sync. Wondering why you switched from using the slightly slower, but less error prone way of passing the viewport attributes? processViewport("width=device-width, initial-scale=1");

> Source/WebCore/dom/Document.cpp:778
> +            && m_docType->publicId().startsWith("-//wapforum//dtd xhtml mobile 1.", /* caseSensitive */ false)) {

Missing your proposal of putting ASSERT here. I think it would be great to have ASSERT(m_viewportArgument.type == Implicit), now that check is removed.

> Source/WebCore/dom/ViewportArguments.h:61
> +        XHTMLMobileProfile,

is it still needed?
------- Comment #57 From 2012-05-09 11:45:55 PST -------
(In reply to comment #56)
> (From update of attachment 140971 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140971&action=review
> 
> > Source/WebCore/dom/Document.cpp:777
> > +        if (!ownerElement()
> 
> Aren't this and the mainframe() check at updateViewportArguments() supposed to serve the same purpose? (that we only do meta viewport for mainframes) While they are probably in sync functionality wise at this point(!ownerelement() vs. page()->mainFrame() == frame()), it is somewhat error prone setup and can result in a situation, where the m_viewportArguments value is changed, but it's not dispatched further and the actual viewport ends being up out of sync. Wondering why you switched from using the slightly slower, but less error prone way of passing the viewport attributes? processViewport("width=device-width, initial-scale=1");
> 
> > Source/WebCore/dom/Document.cpp:778
> > +            && m_docType->publicId().startsWith("-//wapforum//dtd xhtml mobile 1.", /* caseSensitive */ false)) {
> 
> Missing your proposal of putting ASSERT here. I think it would be great to have ASSERT(m_viewportArgument.type == Implicit), now that check is removed.

I changed from the processViewport version to be able to set "m_viewportArgument.type" to XHTMLMobileProfile before a call for updateViewportArguments occur.

I'm still doing this m_viewportArgument.type stuff to be in sync with the patch for bug 55874 that I think would be landed soon, but I think I'm doing it wrong and not following the rule: "Separate things, separate changes" and the result is a patch with non sense things that will only make sense only on future patches.

I'm going to fix this and keep just the essential on the patch, thanks!

> > Source/WebCore/dom/ViewportArguments.h:61
> > +        XHTMLMobileProfile,
> 
> is it still needed?

Yes, but no =], already replied in the comments above.
------- Comment #58 From 2012-05-09 11:49:09 PST -------
> > > Source/WebCore/dom/ViewportArguments.h:61
> > > +        XHTMLMobileProfile,
> > 
> > is it still needed?
> 
> Yes, but no =], already replied in the comments above.

It will be required when we add support for the other legacy meta tags: https://bugs.webkit.org/show_bug.cgi?id=55874
------- Comment #59 From 2012-05-09 11:50:56 PST -------
(In reply to comment #58)
> > > > Source/WebCore/dom/ViewportArguments.h:61
> > > > +        XHTMLMobileProfile,
> > > 
> > > is it still needed?
> > 
> > Yes, but no =], already replied in the comments above.
> 
> It will be required when we add support for the other legacy meta tags: https://bugs.webkit.org/show_bug.cgi?id=55874

Or not, if we consider it as implicit as well, due to the lack of an actual meta tag. I guess that is fine.
------- Comment #60 From 2012-05-09 11:53:49 PST -------
(In reply to comment #58)
> > > > Source/WebCore/dom/ViewportArguments.h:61
> > > > +        XHTMLMobileProfile,
> > > 
> > > is it still needed?
> > 
> > Yes, but no =], already replied in the comments above.
> 
> It will be required when we add support for the other legacy meta tags: https://bugs.webkit.org/show_bug.cgi?id=55874

Let's add it, when it is actually used.
------- Comment #61 From 2012-05-09 12:05:22 PST -------
Created an attachment (id=140991) [details]
Patch
------- Comment #62 From 2012-05-09 12:21:22 PST -------
(From update of attachment 140991 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=140991&action=review

This looks good to me and should be OK to land given it is kept behind a feature flag and it already specified (though indirectly) in the spec. Please also fix the wording before landing.

> LayoutTests/fast/viewport/viewport-legacy-xhtmlmp-misplaced.html:14
> +    However XHTML-MP is a legacy way of setting the viewport, so the viewport
> +    metatag takes precedence.

so any viewport related meta tag takes precedence.

> LayoutTests/fast/viewport/viewport-legacy-xhtmlmp-misplaced.html:16
> +    This test if a missplaced XHTML-MP doctype entry doesn't change the already

This test checks that ... does not viewport.
------- Comment #63 From 2012-05-09 12:26:31 PST -------
(From update of attachment 140991 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=140991&action=review

LGTM

> LayoutTests/fast/viewport/viewport-legacy-xhtmlmp-misplaced.html:1
> +<?xml version="1.0" encoding="utf-8"?>

-misplaced-doctype

> LayoutTests/platform/mac/test_expectations.txt:6
> +SKIP WONTFIX : fast/viewport/viewport-legacy-xhtmlmp.html = FAIL

/fast/viewport is already in skiplist
------- Comment #64 From 2012-05-09 12:44:42 PST -------
(In reply to comment #63)
> (From update of attachment 140991 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140991&action=review
> 
> LGTM
> 
> > LayoutTests/fast/viewport/viewport-legacy-xhtmlmp-misplaced.html:1
> > +<?xml version="1.0" encoding="utf-8"?>
> 
> -misplaced-doctype

The word "xhtmlmp" already refer to doctype, but I can rename the file anyway.

> > LayoutTests/platform/mac/test_expectations.txt:6
> > +SKIP WONTFIX : fast/viewport/viewport-legacy-xhtmlmp.html = FAIL
> 
> /fast/viewport is already in skiplist

Good, so I'll remove this line.
------- Comment #65 From 2012-05-09 12:59:37 PST -------
Created an attachment (id=141002) [details]
Patch
------- Comment #66 From 2012-05-09 13:12:55 PST -------
Me on this bug: http://webkitmemes.tumblr.com/post/18428132908/freshman-webkit-committer-needs-final-approval

Sorry =]
------- Comment #67 From 2012-05-09 14:29:02 PST -------
(From update of attachment 141002 [details])
Rejecting attachment 141002 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/12665026
------- Comment #68 From 2012-05-09 14:40:16 PST -------
Created an attachment (id=141017) [details]
Patch

Now with the reviewer name.
------- Comment #69 From 2012-05-09 16:09:38 PST -------
(From update of attachment 141017 [details])
Clearing flags on attachment: 141017

Committed r116571: <http://trac.webkit.org/changeset/116571>
------- Comment #70 From 2012-05-09 16:09:49 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #71 From 2012-05-10 00:16:58 PST -------
(From update of attachment 141017 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=141017&action=review

> Source/WebCore/dom/Document.cpp:781
> +#if USE(LEGACY_VIEWPORT_ADAPTION)
> +        ASSERT(m_viewportArgument.type == Implicit);
> +        if (m_docType->publicId().startsWith("-//wapforum//dtd xhtml mobile 1.", /* caseSensitive */ false))
> +            processViewport("width=device-width, height=device-height, initial-scale=1");
> +#endif
> +    }

Reopen, because it broke Qt debug builds:
../../../../Source/WebCore/dom/Document.cpp: In member function 'void WebCore::Document::setDocType(WTF::PassRefPtr<WebCore::DocumentType>)':
../../../../Source/WebCore/dom/Document.cpp:777: error: 'm_viewportArgument' was not declared in this scope
../../../../Source/WebCore/dom/Document.cpp:777: error: 'Implicit' was not declared in this scope
------- Comment #72 From 2012-05-10 00:17:25 PST -------
.
------- Comment #73 From 2012-05-10 00:19:25 PST -------
> Reopen, because it broke Qt debug builds:

This is not a good reason to reopen a fixed bug. Haven't we discussed this very recently?

It's appropriate to reopen a bug if a patch is rolled out, but not for follow up discussion.
------- Comment #74 From 2012-05-10 00:24:17 PST -------
(In reply to comment #73)
> > Reopen, because it broke Qt debug builds:
> 
> This is not a good reason to reopen a fixed bug. Haven't we discussed this very recently?
> 
> It's appropriate to reopen a bug if a patch is rolled out, but not for follow up discussion.

I think the broken build is a good reason, because the bug is unfinished.
------- Comment #75 From 2012-05-10 00:30:07 PST -------
You have been told by several reviewers that it's not a good reason. Please stop spamming Bugzilla.
------- Comment #76 From 2012-05-10 00:41:27 PST -------
(In reply to comment #71)
> (From update of attachment 141017 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=141017&action=review
> 
> > Source/WebCore/dom/Document.cpp:781
> > +#if USE(LEGACY_VIEWPORT_ADAPTION)
> > +        ASSERT(m_viewportArgument.type == Implicit);
> > +        if (m_docType->publicId().startsWith("-//wapforum//dtd xhtml mobile 1.", /* caseSensitive */ false))
> > +            processViewport("width=device-width, height=device-height, initial-scale=1");
> > +#endif
> > +    }
> 
> Reopen, because it broke Qt debug builds:
> ../../../../Source/WebCore/dom/Document.cpp: In member function 'void WebCore::Document::setDocType(WTF::PassRefPtr<WebCore::DocumentType>)':
> ../../../../Source/WebCore/dom/Document.cpp:777: error: 'm_viewportArgument' was not declared in this scope
> ../../../../Source/WebCore/dom/Document.cpp:777: error: 'Implicit' was not declared in this scope

Fix landed in http://trac.webkit.org/changeset/116616
------- Comment #77 From 2012-05-10 02:42:49 PST -------
The new assertion hit, I filed a new bug report about it: https://bugs.webkit.org/show_bug.cgi?id=86077

Could you check it, please?