Bug 85425

Summary: Use suitable viewport values when a Mobile DTD is used.
Product: WebKit Reporter: Hugo Parente Lima <hugo.lima>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, ddkilzer, dev+webkit, dglazkov, jkjiang, joepeck, kenneth, kpiascik, lquinn, menard, mlattanzio, ossy, rakuco, tonikitoo, vestbo, webkit.review.bot, zalan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 86077    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
MiniBrowser --window-size 480x720 http://m.yahoo.com (without patch)
none
MiniBrowser --window-size 480x720 http://m.yahoo.com (with patch)
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Hugo Parente Lima 2012-05-02 15:20:40 PDT
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 Hugo Parente Lima 2012-05-02 15:28:52 PDT
Created attachment 139892 [details]
Patch
Comment 2 Alexey Proskuryakov 2012-05-02 17:33:55 PDT
Is this specified in HTML5?
Comment 3 Hugo Parente Lima 2012-05-02 18:18:45 PDT
No, it's just a doctype

http://en.wikipedia.org/wiki/XHTML_Mobile_Profile
Comment 4 zalan 2012-05-02 23:22:56 PDT
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.
Comment 5 zalan 2012-05-02 23:25:33 PDT
and add a test case with mobile dtd to /LayoutTests/fast/viewport
Comment 6 Alexey Proskuryakov 2012-05-02 23:49:42 PDT
> No, it's just a doctype

I don't understand this comment. How does this being a doctype make standardization unnecessary?
Comment 7 Kenneth Rohde Christiansen 2012-05-03 01:43:14 PDT
(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 Kenneth Rohde Christiansen 2012-05-03 01:44:50 PDT
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.
Comment 9 Alexey Proskuryakov 2012-05-03 08:57:24 PDT
Do you have any specific examples of pages that don't look or behave nicely because of this?
Comment 10 Hugo Parente Lima 2012-05-03 10:11:31 PDT
(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 Hugo Parente Lima 2012-05-03 10:15:31 PDT
(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.
Comment 12 Hugo Parente Lima 2012-05-03 10:19:06 PDT
(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.
Comment 13 Hugo Parente Lima 2012-05-03 10:23:21 PDT
(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?
Comment 14 Hugo Parente Lima 2012-05-03 10:24:22 PDT
(In reply to comment #5)
> and add a test case with mobile dtd to /LayoutTests/fast/viewport

Going to do that, thanks.
Comment 15 zalan 2012-05-03 11:15:39 PDT
(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
Comment 16 Hugo Parente Lima 2012-05-03 11:18:49 PDT
(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!
Comment 17 Alexey Proskuryakov 2012-05-03 11:27:47 PDT
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 Hugo Parente Lima 2012-05-03 11:39:48 PDT
(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 zalan 2012-05-03 12:04:56 PDT
(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 Hugo Parente Lima 2012-05-03 12:08:14 PDT
Created attachment 140061 [details]
Patch
Comment 21 Alexey Proskuryakov 2012-05-03 13:08:11 PDT
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 Hugo Parente Lima 2012-05-03 13:11:57 PDT
(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 Hugo Parente Lima 2012-05-03 13:16:16 PDT
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 Alexey Proskuryakov 2012-05-03 13:17:24 PDT
Posting on webkit-dev was what I suggested, yes.
Comment 25 Hugo Parente Lima 2012-05-03 13:18:47 PDT
(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 Kenneth Rohde Christiansen 2012-05-04 05:58:31 PDT
(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 :-)
Comment 27 Antonio Gomes 2012-05-04 07:42:28 PDT
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?
Comment 28 Hugo Parente Lima 2012-05-04 10:46:42 PDT
Created attachment 140272 [details]
MiniBrowser --window-size 480x720 http://m.yahoo.com (without patch)
Comment 29 Hugo Parente Lima 2012-05-04 10:48:24 PDT
Created attachment 140273 [details]
MiniBrowser --window-size 480x720 http://m.yahoo.com (with patch)
Comment 30 Hugo Parente Lima 2012-05-04 11:02:06 PDT
(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 =]
Comment 31 zalan 2012-05-04 12:19:03 PDT
(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)
Comment 32 Ryosuke Niwa 2012-05-04 19:47:59 PDT
Comment on attachment 140061 [details]
Patch

r- because we don't want this feature on desktop browsers. Please add a feature flag.
Comment 33 Kenneth Rohde Christiansen 2012-05-05 05:01:54 PDT
(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.
Comment 34 Kenneth Rohde Christiansen 2012-05-05 05:03:24 PDT
(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 Kenneth Rohde Christiansen 2012-05-08 06:38:52 PDT
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 Hugo Parente Lima 2012-05-08 06:48:21 PDT
(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 Hugo Parente Lima 2012-05-08 10:40:50 PDT
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 Hugo Parente Lima 2012-05-08 12:23:01 PDT
Created attachment 140765 [details]
Patch
Comment 39 Hugo Parente Lima 2012-05-08 12:23:41 PDT
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 Kenneth Rohde Christiansen 2012-05-08 13:02:15 PDT
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
Comment 41 Hugo Parente Lima 2012-05-08 13:49:19 PDT
Created attachment 140772 [details]
Patch
Comment 42 Hugo Parente Lima 2012-05-08 13:57:43 PDT
Created attachment 140775 [details]
Patch
Comment 43 Kenneth Rohde Christiansen 2012-05-08 14:00:16 PDT
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
Comment 44 Hugo Parente Lima 2012-05-08 16:52:20 PDT
Created attachment 140817 [details]
Patch
Comment 45 Build Bot 2012-05-08 17:18:13 PDT
Comment on attachment 140817 [details]
Patch

Attachment 140817 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12643836
Comment 46 WebKit Review Bot 2012-05-08 17:33:22 PDT
Comment on attachment 140817 [details]
Patch

Attachment 140817 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12649548
Comment 47 Kenneth Rohde Christiansen 2012-05-09 02:01:04 PDT
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.
Comment 48 zalan 2012-05-09 02:11:59 PDT
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?
Comment 49 Hugo Parente Lima 2012-05-09 06:39:38 PDT
(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
Comment 50 Hugo Parente Lima 2012-05-09 07:08:40 PDT
Created attachment 140938 [details]
Patch
Comment 51 zalan 2012-05-09 07:14:02 PDT
(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.
Comment 52 Hugo Parente Lima 2012-05-09 07:28:18 PDT
(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.
Comment 53 Alexey Proskuryakov 2012-05-09 09:03:08 PDT
> 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 Hugo Parente Lima 2012-05-09 10:27:49 PDT
Created attachment 140971 [details]
Patch
Comment 55 Hugo Parente Lima 2012-05-09 10:29:09 PDT
(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 zalan 2012-05-09 11:26:23 PDT
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?
Comment 57 Hugo Parente Lima 2012-05-09 11:45:55 PDT
(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.
Comment 58 Kenneth Rohde Christiansen 2012-05-09 11:49:09 PDT
> > > 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 Kenneth Rohde Christiansen 2012-05-09 11:50:56 PDT
(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 zalan 2012-05-09 11:53:49 PDT
(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 Hugo Parente Lima 2012-05-09 12:05:22 PDT
Created attachment 140991 [details]
Patch
Comment 62 Kenneth Rohde Christiansen 2012-05-09 12:21:22 PDT
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.
Comment 63 zalan 2012-05-09 12:26:31 PDT
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
Comment 64 Hugo Parente Lima 2012-05-09 12:44:42 PDT
(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.
Comment 65 Hugo Parente Lima 2012-05-09 12:59:37 PDT
Created attachment 141002 [details]
Patch
Comment 66 Hugo Parente Lima 2012-05-09 13:12:55 PDT
Me on this bug: http://webkitmemes.tumblr.com/post/18428132908/freshman-webkit-committer-needs-final-approval

Sorry =]
Comment 67 WebKit Review Bot 2012-05-09 14:29:02 PDT
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
Comment 68 Hugo Parente Lima 2012-05-09 14:40:16 PDT
Created attachment 141017 [details]
Patch

Now with the reviewer name.
Comment 69 WebKit Review Bot 2012-05-09 16:09:38 PDT
Comment on attachment 141017 [details]
Patch

Clearing flags on attachment: 141017

Committed r116571: <http://trac.webkit.org/changeset/116571>
Comment 70 WebKit Review Bot 2012-05-09 16:09:49 PDT
All reviewed patches have been landed.  Closing bug.
Comment 71 Csaba Osztrogonác 2012-05-10 00:16:58 PDT
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
Comment 72 Csaba Osztrogonác 2012-05-10 00:17:25 PDT
.
Comment 73 Alexey Proskuryakov 2012-05-10 00:19:25 PDT
> 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 Csaba Osztrogonác 2012-05-10 00:24:17 PDT
(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 Alexey Proskuryakov 2012-05-10 00:30:07 PDT
You have been told by several reviewers that it's not a good reason. Please stop spamming Bugzilla.
Comment 76 Csaba Osztrogonác 2012-05-10 00:41:27 PDT
(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
Comment 77 Csaba Osztrogonác 2012-05-10 02:42:49 PDT
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?