Bug 55874

Created attachment 84940 [details] Patch

Attachment 84940 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 Source/WebCore/dom/ViewportArguments.h:113: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 1 in 31 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 84940 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=84940&action=review Awesome patch! <3 Let's see what others think. > Source/WebCore/dom/Document.cpp:2742 > + if (features != "true" || m_viewportArguments.type > type) Checking for "true" should be a case insensitive comparison.

Created attachment 84942 [details] Patch 2

Comment on attachment 84942 [details] Patch 2 View in context: https://bugs.webkit.org/attachment.cgi?id=84942&action=review Nice, I didn't even know about these meta tags. A few questions below. Maybe you can link to a spec in the bugzilla comments if one exists. > Source/WebCore/dom/Document.cpp:2727 > + bool ok; > + m_viewportArguments.width = features.toFloat(&ok); > + > + // An absent value or a value of 0 means that the page width is the same as > + // the size of the screen. We handle invalid values the same way. > + if (features.isEmpty() || features == "0" || !ok) > + m_viewportArguments.width = ViewportArguments::ValueDeviceWidth; Does MobileOptimized have the same numeric parsing as viewport? Should "123x" become 123 or ValueDeviceWidth? > Source/WebCore/dom/Document.cpp:2743 > + // Only override if same type, or if new type has higher priority. > + if (!equalIgnoringCase(features, "true") || m_viewportArguments.type > type) > + return; Should content be trimmed or is this very strict? Would this be valid? <meta name="HandheldFriendly" content=" true">

What is the reason for adding these? If WebKit survived without legacy meta support for so long, won't adding it now cause more trouble than good?

(In reply to comment #6) > What is the reason for adding these? If WebKit survived without legacy meta support for so long, won't adding it now cause more trouble than good? We are actually supporting them in our legacy WebKit based browsers and so does Android. As what I understood from Andreas Kling, iOS actually also inserts a viewport when a XHTML MP document is encountered. We have similar code on our branch: --- a/WebCore/dom/Document.cpp +++ b/WebCore/dom/Document.cpp @@ -661,7 +661,8 @@ void Document::setDocType(PassRefPtr<DocumentType> docType) m_docType->setDocument(this); if (m_docType && !ownerElement() && m_docType->publicId().startsWith("-//wapforum//dtd xhtml mobile 1.", false)) { m_viewportArguments.width = ViewportArguments::ValueDeviceWidth;

(In reply to comment #7) > As what I understood from Andreas Kling, iOS actually also inserts a viewport when a XHTML MP document is encountered. I'm not sure iOS does this for XHTML MP documents, but Android's WebKit does for sure.

(In reply to comment #5) > (From update of attachment 84942 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=84942&action=review > > Nice, I didn't even know about these meta tags. A few questions below. Maybe > you can link to a spec in the bugzilla comments if one exists. I couldn't find all of the docs, but searching a bit I found these: http://msdn.microsoft.com/en-us/library/ms890014.aspx http://blogs.msdn.com/b/iemobile/archive/2010/11/22/the-ie-mobile-viewport-on-windows-phone-7.aspx http://blogs.msdn.com/b/iemobile/archive/2011/01/21/managing-the-browser-viewport-in-windows-phone-7.aspx http://docs.blackberry.com/en/developers/deliverables/6176/HTML_ref_meta_564143_11.jsp http://starkravingfinkle.org/blog/2009/10/fennec-1-0-beta-4-for-maemo/ > > Source/WebCore/dom/Document.cpp:2727 > > + bool ok; > > + m_viewportArguments.width = features.toFloat(&ok); > > + > > + // An absent value or a value of 0 means that the page width is the same as > > + // the size of the screen. We handle invalid values the same way. > > + if (features.isEmpty() || features == "0" || !ok) > > + m_viewportArguments.width = ViewportArguments::ValueDeviceWidth; > > Does MobileOptimized have the same numeric parsing as viewport? Should "123x" become 123 or ValueDeviceWidth? I didn't find this documented any where, but I will try to test it when I get my hand on one of these phones. > > Source/WebCore/dom/Document.cpp:2743 > > + // Only override if same type, or if new type has higher priority. > > + if (!equalIgnoringCase(features, "true") || m_viewportArguments.type > type) > > + return; > > Should content be trimmed or is this very strict? Would this be valid? > > <meta name="HandheldFriendly" content=" true"> I will try to test it. (In reply to comment #6) > What is the reason for adding these? If WebKit survived without legacy meta support for so long, won't adding it now cause more trouble than good? We have had support for these quite some time on our development branches and our testers have reported no regressions so far. Pages that support both viewport and say handheldfriendly, will use the viewport meta tag, independently of the order they are declared.

It's good to know that no regressions have been found yet, although it's unclear if the same would hold true for iOS. Are there any known pages that benefit from this?

(In reply to comment #10) > It's good to know that no regressions have been found yet, although it's unclear if the same would hold true for iOS. Are there any known pages that benefit from this? There are (though I don't have them handy right now) as we got bug reports of some of these, which led me to implement this feature.

Comment on attachment 84942 [details] Patch 2 View in context: https://bugs.webkit.org/attachment.cgi?id=84942&action=review > Source/WebCore/dom/Document.cpp:2712 > + static ViewportArguments::Type type = ViewportArguments::MobileOptimizedMeta; A variable here seems overkill IMHO

Comment on attachment 84942 [details] Patch 2 Patch looks okay to me, but we need clarity on the impact or a list of sites that use this before landing.

So, what about this patch? Can it be landed? The thread about it on www-style[1] is almost dead but has no objections. BTW, the patch needs to be rebased to apply on current WebKit trunk. [1] http://lists.w3.org/Archives/Public/www-style/2012May/0371.html

I think that under the LEGACY_VIEWPORT? flag, landing it would be quite OK. It will also be required for fully upstreaming the Android port.

+johnme Yes, we're also using the MobileOptimized and HandheldFriendly meta tags, however, for the MobileOptimized meta, we completely ignore the content and I don't think we set the initial scale to 1 for HandheldFriendly. This does look like a much more sustainable way than what we have right now. John, could you have a look as well please?

(In reply to comment #16) > +johnme > > Yes, we're also using the MobileOptimized and HandheldFriendly meta tags, however, for the MobileOptimized meta, we completely ignore the content and I don't think we set the initial scale to 1 for HandheldFriendly. Let's try to do the same. Do you have any tests? > This does look like a much more sustainable way than what we have right now. John, could you have a look as well please? John, you don't seem to auto complete in the cc field. Are you in the committers.py?

Created attachment 144626 [details] Patch

I did the requested changes on Kenneth's patch[1] and rebased it. The changes: - Ignore content value of MobileOptimized. - Don't set initial-scale on HandheldFriendly (need to do the same on MobileOptimized?) BTW... height=device-height need to be set? what the behavior on other browsers? [1] Kenneth asked me to do it weeks ago, so it wasn't an intrusive patch =]

(In reply to comment #19) > I did the requested changes on Kenneth's patch[1] and rebased it. > > The changes: > > - Ignore content value of MobileOptimized. > - Don't set initial-scale on HandheldFriendly (need to do the same on MobileOptimized?) > > BTW... height=device-height need to be set? what the behavior on other browsers? > > [1] Kenneth asked me to do it weeks ago, so it wasn't an intrusive patch =] John Mellor recently sent an email to www-style explaining the existing behavior. Maybe you can dig it up? I even think you were cc'ed.

(In reply to comment #20) > (In reply to comment #19) > > I did the requested changes on Kenneth's patch[1] and rebased it. > > > > The changes: > > > > - Ignore content value of MobileOptimized. > > - Don't set initial-scale on HandheldFriendly (need to do the same on MobileOptimized?) > > > > BTW... height=device-height need to be set? what the behavior on other browsers? > > > > [1] Kenneth asked me to do it weeks ago, so it wasn't an intrusive patch =] > > John Mellor recently sent an email to www-style explaining the existing behavior. Maybe you can dig it up? I even think you were cc'ed. Yes I am and read it, but the email is just about widths, not heights.

Comment on attachment 144626 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144626&action=review > Source/WebCore/dom/ViewportArguments.h:62 > +#if USE(LEGACY_VIEWPORT_ADAPTION) This should probably be ENABLE(LEGACY_VIEWPORT_ADAPTION)

Comment on attachment 144626 [details] Patch This looks like a good start at least. We can iterate on the exact parsing rules in followup patches, if needed.

Comment on attachment 144626 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144626&action=review > Source/WebCore/dom/ViewportArguments.h:63 > + XHTMLMobileProfile, XHTMLMP again?..

Comment on attachment 144626 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144626&action=review >> Source/WebCore/dom/ViewportArguments.h:63 >> + XHTMLMobileProfile, > > XHTMLMP again?.. If you look at the patch, you'll see that we're already processing "-//wapforum//dtd xhtml mobile 1." The patch doesn't add support for anything new related to XHTMLMP. It just creates an enum value for what we're already doing.

(In reply to comment #25) > (From update of attachment 144626 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=144626&action=review > > >> Source/WebCore/dom/ViewportArguments.h:63 > >> + XHTMLMobileProfile, > > > > XHTMLMP again?.. > > If you look at the patch, you'll see that we're already processing "-//wapforum//dtd xhtml mobile 1." The patch doesn't add support for anything new related to XHTMLMP. It just creates an enum value for what we're already doing. Yep, and the enum is needed just to know when processViewport should overwrite the current viewport settings or not.

(In reply to comment #22) > (From update of attachment 144626 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=144626&action=review > > > Source/WebCore/dom/ViewportArguments.h:62 > > +#if USE(LEGACY_VIEWPORT_ADAPTION) > > This should probably be ENABLE(LEGACY_VIEWPORT_ADAPTION) The "USE" was suggested and included in a previous patch, so this patch just continue using USE instead of ENABLE. Anyway I think the semantic difference between both isn't to big and IMO webkit would have just "USE" or just "ENABLE" to keep things simple.

Comment on attachment 144626 [details] Patch Clearing flags on attachment: 144626 Committed r119256: <http://trac.webkit.org/changeset/119256>

All reviewed patches have been landed. Closing bug.

> The "USE" was suggested and included in a previous patch, so this patch just continue using USE instead of ENABLE. > > Anyway I think the semantic difference between both isn't to big and IMO webkit would have just "USE" or just "ENABLE" to keep things simple. Yeah, the two are very similar, and it's not clear what we gain by differentiating them. My understanding is that USE is for selecting among different ways of implementing features (e.g., which library to use or whether to use an in-memory cache) whereas ENABLE is about controlling which features are visible to the platform (e.g., IndexedDB or various meta tags). It's possible that other folks have a different understanding, and it's certainly not a big deal one way or the other.

(In reply to comment #30) > > The "USE" was suggested and included in a previous patch, so this patch just continue using USE instead of ENABLE. > > > > Anyway I think the semantic difference between both isn't to big and IMO webkit would have just "USE" or just "ENABLE" to keep things simple. > > Yeah, the two are very similar, and it's not clear what we gain by differentiating them. My understanding is that USE is for selecting among different ways of implementing features (e.g., which library to use or whether to use an in-memory cache) whereas ENABLE is about controlling which features are visible to the platform (e.g., IndexedDB or various meta tags). > > It's possible that other folks have a different understanding, and it's certainly not a big deal one way or the other. I can do the switch to ENABLE in another patch as well and add you as reviewer. Regards

> I can do the switch to ENABLE in another patch as well and add you as reviewer. Thanks Hugo.

Sorry, missed this. Belated review: looks great, thanks for adding support for these, and seems a nice clean way to do priorities. It seems a bit odd that depending on the source we use "width=device-width, height=device-height", "width=device-width" or "width=device-width, initial-scale=1" as the implied viewport tag. Would anyone object if I changed these all to be just "width=device-width", on the assumption that initial-scale and device-height both default to those values once width=device-width is set?

John, you might want to open a new bug and attach a patch. That way folks can comment without feeling like they need to read 33 previous comments.

I don't mind.