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
Hugo Parente Lima
2012-05-02 15:20:40 PDT
Created attachment 139892 [details]
Patch
Is this specified in HTML5? No, it's just a doctype http://en.wikipedia.org/wiki/XHTML_Mobile_Profile 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. and add a test case with mobile dtd to /LayoutTests/fast/viewport > No, it's just a doctype
I don't understand this comment. How does this being a doctype make standardization unnecessary?
(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 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. Do you have any specific examples of pages that don't look or behave nicely because of this? (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. (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. (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. (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? (In reply to comment #5) > and add a test case with mobile dtd to /LayoutTests/fast/viewport Going to do that, thanks. (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 (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! 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). (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. (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> Created attachment 140061 [details]
Patch
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. (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? 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. Posting on webkit-dev was what I suggested, yes. (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. (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 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?
Created attachment 140272 [details] MiniBrowser --window-size 480x720 http://m.yahoo.com (without patch) Created attachment 140273 [details] MiniBrowser --window-size 480x720 http://m.yahoo.com (with patch) (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 =] (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 on attachment 140061 [details]
Patch
r- because we don't want this feature on desktop browsers. Please add a feature flag.
(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. (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. 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." (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? 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 Created attachment 140765 [details]
Patch
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 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 Created attachment 140772 [details]
Patch
Created attachment 140775 [details]
Patch
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 Created attachment 140817 [details]
Patch
Comment on attachment 140817 [details] Patch Attachment 140817 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12643836 Comment on attachment 140817 [details] Patch Attachment 140817 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12649548 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 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? (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 Created attachment 140938 [details]
Patch
(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. (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. > 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. Created attachment 140971 [details]
Patch
(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 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? (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. > > > 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 (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. (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. Created attachment 140991 [details]
Patch
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 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 (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. Created attachment 141002 [details]
Patch
Me on this bug: http://webkitmemes.tumblr.com/post/18428132908/freshman-webkit-committer-needs-final-approval Sorry =] 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 Created attachment 141017 [details]
Patch
Now with the reviewer name.
Comment on attachment 141017 [details] Patch Clearing flags on attachment: 141017 Committed r116571: <http://trac.webkit.org/changeset/116571> All reviewed patches have been landed. Closing bug. 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 . > 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.
(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. You have been told by several reviewers that it's not a good reason. Please stop spamming Bugzilla. (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 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? |