Bug 85425 – Use suitable viewport values when a Mobile DTD is used.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Hugo Parente Lima]

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.

[Hugo Parente Lima]

Created attachment 139892 [details]
Patch

[Alexey Proskuryakov]

Is this specified in HTML5?

[Hugo Parente Lima]

No, it's just a doctype

http://en.wikipedia.org/wiki/XHTML_Mobile_Profile

[zalan]

Comment on attachment 139892 [details]
Patch

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.

[zalan]

and add a test case with mobile dtd to /LayoutTests/fast/viewport

[Alexey Proskuryakov]

> No, it's just a doctype

I don't understand this comment. How does this being a doctype make standardization unnecessary?

[Kenneth Rohde Christiansen]

(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.

[Kenneth Rohde Christiansen]

Comment on attachment 139892 [details]
Patch

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.

[Alexey Proskuryakov]

Do you have any specific examples of pages that don't look or behave nicely because of this?

[Hugo Parente Lima]

(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.

[Hugo Parente Lima]

(In reply to comment #8)
> (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?

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.

[Hugo Parente Lima]

(In reply to comment #4)
> (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.

Seems feasible not changing the WebCore::Document API despite of being a bit slower, but I doubt the difference would be notifiable.

[Hugo Parente Lima]

(In reply to comment #8)
> (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.

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?

[Hugo Parente Lima]

(In reply to comment #5)
> and add a test case with mobile dtd to /LayoutTests/fast/viewport

Going to do that, thanks.

[zalan]

(In reply to comment #11)
> (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.

n9's webkit branch is on gitorious.
https://gitorious.org/+qtwebkit-webkit2-dev/webkit/qtwebkit-webkit2-dev/blobs/master/WebCore/dom/Document.cpp

[Hugo Parente Lima]

(In reply to comment #15)
> (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

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!

[Alexey Proskuryakov]

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).

[Hugo Parente Lima]

(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.

[zalan]

(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>

[Hugo Parente Lima]

Created attachment 140061 [details]
Patch

[Alexey Proskuryakov]

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.

[Hugo Parente Lima]

(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?

[Hugo Parente Lima]

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.

[Alexey Proskuryakov]

Posting on webkit-dev was what I suggested, yes.

[Hugo Parente Lima]

(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.

[Kenneth Rohde Christiansen]

(In reply to comment #11)
> (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.

It is on gitorious :-)

[Antonio Gomes]

Comment on attachment 140061 [details]
Patch

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?

[Hugo Parente Lima]

Created attachment 140272 [details]
MiniBrowser --window-size 480x720 http://m.yahoo.com (without patch)

[Hugo Parente Lima]

Created attachment 140273 [details]
MiniBrowser --window-size 480x720 http://m.yahoo.com (with patch)

[Hugo Parente Lima]

(In reply to comment #27)
> (From update of attachment 140061 [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 =]

[zalan]

(In reply to comment #27)
> (From update of attachment 140061 [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)

[Ryosuke Niwa]

Comment on attachment 140061 [details]
Patch

r- because we don't want this feature on desktop browsers. Please add a feature flag.

[Kenneth Rohde Christiansen]

(In reply to comment #32)
> (From update of attachment 140061 [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.

[Kenneth Rohde Christiansen]

(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.

[Kenneth Rohde Christiansen]

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."

[Hugo Parente Lima]

(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?

[Hugo Parente Lima]

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

[Hugo Parente Lima]

Created attachment 140765 [details]
Patch

[Hugo Parente Lima]

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/*.

[Kenneth Rohde Christiansen]

Comment on attachment 140765 [details]
Patch

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

[Hugo Parente Lima]

Created attachment 140772 [details]
Patch

[Hugo Parente Lima]

Created attachment 140775 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 140772 [details]
Patch

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

[Hugo Parente Lima]

Created attachment 140817 [details]
Patch

[Build Bot]

Comment on attachment 140817 [details]
Patch

Attachment 140817 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12643836

[WebKit Review Bot]

Comment on attachment 140817 [details]
Patch

Attachment 140817 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12649548

[Kenneth Rohde Christiansen]

Comment on attachment 140817 [details]
Patch

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.

[zalan]

Comment on attachment 140817 [details]
Patch

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?

[Hugo Parente Lima]

(In reply to comment #48)
> (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?

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

[Hugo Parente Lima]

Created attachment 140938 [details]
Patch

[zalan]

(In reply to comment #49)
> (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
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.

[Hugo Parente Lima]

(In reply to comment #51)
> (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.

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

[Alexey Proskuryakov]

> 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.

[Hugo Parente Lima]

Created attachment 140971 [details]
Patch

[Hugo Parente Lima]

(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.

[zalan]

Comment on attachment 140971 [details]
Patch

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?

[Hugo Parente Lima]

(In reply to comment #56)
> (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.

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.

[Kenneth Rohde Christiansen]

> > > 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

[Kenneth Rohde Christiansen]

(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.

[zalan]

(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.

[Hugo Parente Lima]

Created attachment 140991 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 140991 [details]
Patch

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.

[zalan]

Comment on attachment 140991 [details]
Patch

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

[Hugo Parente Lima]

(In reply to comment #63)
> (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

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.

[Hugo Parente Lima]

Created attachment 141002 [details]
Patch

[Hugo Parente Lima]

Me on this bug: http://webkitmemes.tumblr.com/post/18428132908/freshman-webkit-committer-needs-final-approval

Sorry =]

[WebKit Review Bot]

Comment on attachment 141002 [details]
Patch

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

[Hugo Parente Lima]

Created attachment 141017 [details]
Patch

Now with the reviewer name.

[WebKit Review Bot]

Comment on attachment 141017 [details]
Patch

Clearing flags on attachment: 141017

Committed r116571: <http://trac.webkit.org/changeset/116571>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

Comment on attachment 141017 [details]
Patch

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

[Csaba Osztrogonác]

.

[Alexey Proskuryakov]

> 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.

[Csaba Osztrogonác]

(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.

[Alexey Proskuryakov]

You have been told by several reviewers that it's not a good reason. Please stop spamming Bugzilla.

[Csaba Osztrogonác]

(In reply to comment #71)
> (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

Fix landed in http://trac.webkit.org/changeset/116616

[Csaba Osztrogonác]

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?