Bug 39902 – [Qt] QtWebKit does not support viewport meta tag

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Jesus Sanchez-Palencia]

QtWebKit does not support the viewport meta tag, which is very useful for mobile browsing.
The ground work for supporting this is already in WebCore::ChromeClient so we just need to implement the needed functions and adapt our API layer.

[Jesus Sanchez-Palencia]

Created attachment 57386 [details]
patch

[WebKit Review Bot]

Attachment 57386 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKit/qt/WebCoreSupport/ChromeClientQt.cpp:66:  Alphabetical sorting problem.  [build/include_order] [4]
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebpage.cpp"
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebpage.h"
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebframe_p.h"
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jesus Sanchez-Palencia]

Me and Kenneth add support to yberbrowser as an example of what is needed to get this working on a QtWebKit browser.

http://gitorious.org/~jeez/yberbrowser/jeez-yberbrowser/commit/0a7df040318cd83255a4360b1af52924129743d5

[Jesus Sanchez-Palencia]

Created attachment 57387 [details]
patch

[Kenneth Rohde Christiansen]

Documents of interest:

http://hacks.mozilla.org/2010/05/upcoming-changes-to-the-viewport-meta-tag-for-firefox-mobile/
http://www.quirksmode.org/blog/archives/2010/04/a_pixel_is_not.html
http://developer.apple.com/safari/library/documentation/appleapplications/reference/safarihtmlref/articles/metatags.html

[Simon Hausmann]

Comment on attachment 57387 [details]
patch

WebKit/qt/Api/qwebpage.h:306
 +      virtual void prepareViewportChange(QSize contentsViewportSize, qreal initialScale, qreal minAllowedScale, qreal maxAllowedScale);
Unfortunately we cannot add virtual functions without breaking binary compatibility :(

[Simon Hausmann]

I wonder if it would make sense to split up the patch. It covers three parts, viewport meta support, fixing windowRect() and fixing the docs of setPreferredContentsSize (great, btw :)

I suggest to replace the virtual method with a signal and explain a bit more _how_ to use the provided values, i.e. with sample code. The current docs are very detailed but it's not very clear what to do with the values. For example "the initial scale should be applied"  could be phrased as that this is the scale the browser should apply for the zoom.

Put yourself into the seat of a third-party developer and try to understand what to do with the signal :-)

[Kenneth Rohde Christiansen]

(In reply to comment #7)
> I wonder if it would make sense to split up the patch. It covers three parts, viewport meta support, fixing windowRect() and fixing the docs of setPreferredContentsSize (great, btw :)
> 
> I suggest to replace the virtual method with a signal and explain a bit more _how_ to use the provided values, i.e. with sample code. The current docs are very detailed but it's not very clear what to do with the values. For example "the initial scale should be applied"  could be phrased as that this is the scale the browser should apply for the zoom.
> 
> Put yourself into the seat of a third-party developer and try to understand what to do with the signal :-)

I agree it makes sense splitting it up, but we didn't do so at first as all the changes are somewhat related. I'm sure Jesus can do this on monday.

The whole viewport meta tag is utterly complicated for developers, even for those creating mobile webkit based browsers or the fennec developers :-) What we really need is some documentation for web developers as well that we can refer to, something along with what Apple have for the iphone version of Safari.

I will see if we can take the sample code from yberbrowser and provide some more elaborate documentation. I guess that could be done as a follow up patch.

[Simon Hausmann]

(In reply to comment #8)
> (In reply to comment #7)
> > I wonder if it would make sense to split up the patch. It covers three parts, viewport meta support, fixing windowRect() and fixing the docs of setPreferredContentsSize (great, btw :)
> > 
> > I suggest to replace the virtual method with a signal and explain a bit more _how_ to use the provided values, i.e. with sample code. The current docs are very detailed but it's not very clear what to do with the values. For example "the initial scale should be applied"  could be phrased as that this is the scale the browser should apply for the zoom.
> > 
> > Put yourself into the seat of a third-party developer and try to understand what to do with the signal :-)
> 
> I agree it makes sense splitting it up, but we didn't do so at first as all the changes are somewhat related. I'm sure Jesus can do this on monday.

Don't worry, I see the intention now. It's probably not worth the split up :)

Awesome work :)

> The whole viewport meta tag is utterly complicated for developers, even for those creating mobile webkit based browsers or the fennec developers :-) What we really need is some documentation for web developers as well that we can refer to, something along with what Apple have for the iphone version of Safari.
> 
> I will see if we can take the sample code from yberbrowser and provide some more elaborate documentation. I guess that could be done as a follow up patch.

Yeah

[Jesus Sanchez-Palencia]

Created attachment 57465 [details]
patch

Patch fixed. The virtual function was replaced by a signal.

Also the patch to yberbrowser was updated. The new one to be used as an example is here: http://gitorious.org/~jeez/yberbrowser/jeez-yberbrowser/commit/2180551ef8e2525c8dec4a6b2a45b249013c5ab7

[Jesus Sanchez-Palencia]

Created attachment 57473 [details]
patch

Now using a const QSize& and added the user-scalable argument.

Follow up updated patch to yberbrowser: http://gitorious.org/~jeez/yberbrowser/jeez-yberbrowser/commit/9f6bd7b1069d0806904403c98c3637e7ba1a8f71

[Kenneth Rohde Christiansen]

I would prefer isUserScalable, sounds more Qt'ish.

[Jesus Sanchez-Palencia]

Created attachment 57704 [details]
Patch

Now using a struct: QWebPage::ViewportHints.

[Simon Hausmann]

Comment on attachment 57704 [details]
Patch

WebKit/qt/Api/qwebpage.cpp:3558
 +      \fn void QWebPage::viewportChangeRequested(const ViewportHints& hints)
The signature in the documentation differs from the signature in the header file, the QWebPage:: class prefix is missing for the ViewportHints type argument.

[Simon Hausmann]

Looking for more API feedback with https://lists.webkit.org/pipermail/webkit-qt/2010-June/000577.html before proceeding with the review.

[Richard Moore]

Having a flag value of -1.0 for invalid values is a bad plan since floating point equality cannot be tested properly - going this way would require everyone to start messing around with EPSILON etc. Are there any valid use-cases for values less than or equal to 0? If not then why not simply say that negative values are the invalid ones.

[Kenneth Rohde Christiansen]

(In reply to comment #16)
> Having a flag value of -1.0 for invalid values is a bad plan since floating point equality cannot be tested properly - going this way would require everyone to start messing around with EPSILON etc. Are there any valid use-cases for values less than or equal to 0? If not then why not simply say that negative values are the invalid ones.

Internally in WebKit we are given -1, but I guess we could change the documentation. I do not know if there are any better ways for dealing with these invalid values in a Qt'ish fashion

[Simon Hausmann]

Comment on attachment 57704 [details]
Patch

Ok, let's continue with the review.

WebKit/qt/Api/qwebframe_p.h:102
 +      bool hasPassedLayoutPhase;
I kind of like the term "initialLayoutComplete", similar to void FrameLoaderClientQt::dispatchDidFirstLayout()

AFAICS this is entirely an optimization to avoid a double initial layout, right?

WebKit/qt/Api/qwebpage.cpp:2213
 +      Setting an invalid size, makes the page fall back to using the viewport for layout.
Suggestion: "using the viewport for layout" -> "using the viewport size for layout"

WebKit/qt/Api/qwebpage.h:197
 +      struct ViewportHints {
I suggest to make the constructor non-inline, prefix the member variables with m_, provide inline getters, add a non-inline destructor, copy constructor and assignment operator. In addition I suggest to add a ViewportHintsPrivate* d; for future-proofing.

So that by default the structure is created using one function call and one initial allocation, but we _can_ extend it in the future without breaking binary or source compatibility.

WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:397
 +      emit m_webFrame->page()->viewportChangeRequested(QWebPage::ViewportHints());
Hmm, this is used to "reset" the scale in the browser, right?

It's kind of ugly. I guess it could be made prettier if the docs of the signal explain that this is going to happen and we could have a isValid() or so in the structure.

[Jesus Sanchez-Palencia]

(In reply to comment #18)
> (From update of attachment 57704 [details])
> Ok, let's continue with the review.

Thanks! I'll address all comments and upload a new patch.

[Jesus Sanchez-Palencia]

Created attachment 58190 [details]
patch

Addressed all previous comments.

[Kenneth Rohde Christiansen]

+        , isUserScalable(-1.0)

Isn't this supposed to be a bool?

[Jesus Sanchez-Palencia]

Created attachment 58237 [details]
patch

[Ariya Hidayat]

Comment on attachment 58237 [details]
patch

Sorry for jumping late.

> +qreal QWebPage::ViewportHints::initialScale() const
> +qreal QWebPage::ViewportHints::minimumScale() const
> +qreal QWebPage::ViewportHints::maximumScale() const

Just a minor note: should we use "scaling factor" instead? "scale" easily confuses some people because it is also a verb (implies an action), while "scaling factor" is guaranteed to sound like a property.

For an analogy, we already have "zoom factor" for QWebFrame and QWebView.

[Jesus Sanchez-Palencia]

Created attachment 58392 [details]
patch

Rebased and using {initial, minimum, maximum}ScaleFactor() instead of {initial, minimum, maximum}Scale().

[WebKit Review Bot]

Attachment 58392 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKit/qt/WebCoreSupport/ChromeClientQt.cpp:45:  Alphabetical sorting problem.  [build/include_order] [4]
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebframe_p.h"
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebpage_p.h"
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebpage.h"
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebpage.cpp"
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jesus Sanchez-Palencia]

Created attachment 58393 [details]
patch

fixed include order.

[Simon Hausmann]

(In reply to comment #26)
> Created an attachment (id=58393) [details]
> patch

I think the existing members should be inline, so that by default allocating the ViewportHints structure does _not_ result in an additional and expensive malloc(). But we should keep the private d-pointer for future expansion.

[Jesus Sanchez-Palencia]

(In reply to comment #27)
> I think the existing members should be inline, so that by default allocating the ViewportHints structure does _not_ result in an additional and expensive malloc(). But we should keep the private d-pointer for future expansion.

Thanks for the comment! :)

Do you mean inline getters as you've asked before? I didn't add those because if I did I'd have to "expose" the private class by including it on the public header since a forward declaration wouldn't be enough.

Having the members themselves on the public class is one solution, but then by doing this aren't we being too much confident that the viewport meta tag spec won't change in the future? Removing or adding members to the public structure later would break binary compatibility, right?

[Simon Hausmann]

(In reply to comment #28)
> (In reply to comment #27)
> > I think the existing members should be inline, so that by default allocating the ViewportHints structure does _not_ result in an additional and expensive malloc(). But we should keep the private d-pointer for future expansion.
> 
> Thanks for the comment! :)
> 
> Do you mean inline getters as you've asked before? I didn't add those because if I did I'd have to "expose" the private class by including it on the public header since a forward declaration wouldn't be enough.
> 
> Having the members themselves on the public class is one solution, but then by doing this aren't we being too much confident that the viewport meta tag spec won't change in the future? Removing or adding members to the public structure later would break binary compatibility, right?

Indeed we cannot remove fields, but if we want to add fields, then we can being using the d-pointer.

[Jesus Sanchez-Palencia]

Created attachment 58699 [details]
patch

Addressed the previous comments.

[Simon Hausmann]

(In reply to comment #23)
> (From update of attachment 58237 [details])
> Sorry for jumping late.
> 
> > +qreal QWebPage::ViewportHints::initialScale() const
> > +qreal QWebPage::ViewportHints::minimumScale() const
> > +qreal QWebPage::ViewportHints::maximumScale() const
> 
> Just a minor note: should we use "scaling factor" instead? "scale" easily confuses some people because it is also a verb (implies an action), while "scaling factor" is guaranteed to sound like a property.
> 
> For an analogy, we already have "zoom factor" for QWebFrame and QWebView.

On the other hand we have QGraphicsItem::scale() :-)

[Simon Hausmann]

Comment on attachment 58699 [details]
patch

WebKit/qt/Api/qwebpage.cpp:1681
 +      : d(new QtViewportHintsPrivate(this))
Almost :) Since we're not using it currently, we can save some cycles and initialize d to 0 here.

WebKit/qt/Api/qwebpage.cpp:1694
 +  QWebPage::ViewportHints::ViewportHints(const QWebPage::ViewportHints& other)
This copy constructor must also copy the member variables.

WebKit/qt/Api/qwebpage.cpp:1712
 +  QWebPage::ViewportHints& QWebPage::ViewportHints::operator=(const QWebPage::ViewportHints& other)
Same here, the member variables need to be copied.

[Jesus Sanchez-Palencia]

Created attachment 58922 [details]
patch

Added the missing members copying and assignments. :)

[Simon Hausmann]

Comment on attachment 58922 [details]
patch

r=me

[Jesus Sanchez-Palencia]

Committed r61342: <http://trac.webkit.org/changeset/61342>

[Jesus Sanchez-Palencia]

Comment on attachment 58922 [details]
patch

Clearing cq flag.

[Anand Patil]

Is this patch getting into QtWebkit 2.0?

[Kenneth Rohde Christiansen]

Do you need it?

[Anand Patil]

(In reply to comment #38)
> Do you need it?

Oh yes!

[Simon Hausmann]

In the context of the impending release, the rapidly shrinking list of critical bugs I suggest we do not include this feature in the release.