Full Text Bug Listing

Hugo Parente Lima

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

Hugo Parente Lima

Comment 1 2012-05-02 15:28:52 PDT

Created attachment 139892 [details] Patch

Alexey Proskuryakov

Comment 2 2012-05-02 17:33:55 PDT

Is this specified in HTML5?

Hugo Parente Lima

Comment 3 2012-05-02 18:18:45 PDT

No, it's just a doctype http://en.wikipedia.org/wiki/XHTML_Mobile_Profile

alan

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

alan

Comment 5 2012-05-02 23:25:33 PDT

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

Alexey Proskuryakov

Comment 6 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?

Kenneth Rohde Christiansen

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

Kenneth Rohde Christiansen

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

Alexey Proskuryakov

Comment 9 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?

Hugo Parente Lima

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

Hugo Parente Lima

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

Hugo Parente Lima

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

Hugo Parente Lima

Comment 13 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?

Hugo Parente Lima

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

alan

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

Hugo Parente Lima

Comment 16 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!

Alexey Proskuryakov

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

Hugo Parente Lima

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

alan

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

Hugo Parente Lima

Comment 20 2012-05-03 12:08:14 PDT

Created attachment 140061 [details] Patch

Alexey Proskuryakov

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

Hugo Parente Lima

Comment 22 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?

Hugo Parente Lima

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

Alexey Proskuryakov

Comment 24 2012-05-03 13:17:24 PDT

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

Hugo Parente Lima

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

Kenneth Rohde Christiansen

Comment 26 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 :-)

Antonio Gomes

Comment 27 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?

Hugo Parente Lima

Comment 28 2012-05-04 10:46:42 PDT

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

Hugo Parente Lima

Comment 29 2012-05-04 10:48:24 PDT

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

Hugo Parente Lima

Comment 30 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 =]

alan

Comment 31 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)

Ryosuke Niwa

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

Kenneth Rohde Christiansen

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

Kenneth Rohde Christiansen

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

Kenneth Rohde Christiansen

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

Hugo Parente Lima

Comment 36 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?

Hugo Parente Lima

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

Hugo Parente Lima

Comment 38 2012-05-08 12:23:01 PDT

Created attachment 140765 [details] Patch

Hugo Parente Lima

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

Kenneth Rohde Christiansen

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

Hugo Parente Lima

Comment 41 2012-05-08 13:49:19 PDT

Created attachment 140772 [details] Patch

Hugo Parente Lima

Comment 42 2012-05-08 13:57:43 PDT

Created attachment 140775 [details] Patch

Kenneth Rohde Christiansen

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

Hugo Parente Lima

Comment 44 2012-05-08 16:52:20 PDT

Created attachment 140817 [details] Patch

Build Bot

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

WebKit Review Bot

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

Kenneth Rohde Christiansen

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

alan

Comment 48 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?

Hugo Parente Lima

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

Hugo Parente Lima

Comment 50 2012-05-09 07:08:40 PDT

Created attachment 140938 [details] Patch

alan

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

Hugo Parente Lima

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

Alexey Proskuryakov

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

Hugo Parente Lima

Comment 54 2012-05-09 10:27:49 PDT

Created attachment 140971 [details] Patch

Hugo Parente Lima

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

alan

Comment 56 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?

Hugo Parente Lima

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

Kenneth Rohde Christiansen

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

Kenneth Rohde Christiansen

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

alan

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

Hugo Parente Lima

Comment 61 2012-05-09 12:05:22 PDT

Created attachment 140991 [details] Patch

Kenneth Rohde Christiansen

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

alan

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

Hugo Parente Lima

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

Hugo Parente Lima

Comment 65 2012-05-09 12:59:37 PDT

Created attachment 141002 [details] Patch

Hugo Parente Lima

Comment 66 2012-05-09 13:12:55 PDT

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

WebKit Review Bot

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

Hugo Parente Lima

Comment 68 2012-05-09 14:40:16 PDT

Created attachment 141017 [details] Patch Now with the reviewer name.

WebKit Review Bot

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

WebKit Review Bot

Comment 70 2012-05-09 16:09:49 PDT

All reviewed patches have been landed. Closing bug.

Csaba Osztrogonác

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

Csaba Osztrogonác

Comment 72 2012-05-10 00:17:25 PDT

.

Alexey Proskuryakov

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

Csaba Osztrogonác

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

Alexey Proskuryakov

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

Csaba Osztrogonác

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

Csaba Osztrogonác

Comment 77 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?

Attachments
Patch (3.04 KB, patch) 2012-05-02 15:28 PDT, Hugo Parente Lima	no flags	Details Formatted Diff Diff
Patch (3.72 KB, patch) 2012-05-03 12:08 PDT, Hugo Parente Lima	no flags	Details Formatted Diff Diff
MiniBrowser --window-size 480x720 http://m.yahoo.com (without patch) (92.91 KB, image/png) 2012-05-04 10:46 PDT, 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 PDT, Hugo Parente Lima	no flags	Details
Patch (9.47 KB, patch) 2012-05-08 12:23 PDT, Hugo Parente Lima	no flags	Details Formatted Diff Diff
Patch (10.00 KB, patch) 2012-05-08 13:49 PDT, Hugo Parente Lima	no flags	Details Formatted Diff Diff
Patch (10.01 KB, patch) 2012-05-08 13:57 PDT, Hugo Parente Lima	no flags	Details Formatted Diff Diff
Patch (12.17 KB, patch) 2012-05-08 16:52 PDT, Hugo Parente Lima	no flags	Details Formatted Diff Diff
Patch (13.72 KB, patch) 2012-05-09 07:08 PDT, Hugo Parente Lima	no flags	Details Formatted Diff Diff
Patch (15.97 KB, patch) 2012-05-09 10:27 PDT, Hugo Parente Lima	no flags	Details Formatted Diff Diff
Patch (14.72 KB, patch) 2012-05-09 12:05 PDT, Hugo Parente Lima	no flags	Details Formatted Diff Diff
Patch (14.08 KB, patch) 2012-05-09 12:59 PDT, Hugo Parente Lima	no flags	Details Formatted Diff Diff
Patch (14.04 KB, patch) 2012-05-09 14:40 PDT, Hugo Parente Lima	no flags	Details Formatted Diff Diff
Show Obsolete (10) View All Add attachment proposed patch, testcase, etc.

Bug 85425