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.
Created attachment 57386 [details] patch
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.
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
Created attachment 57387 [details] patch
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
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 :(
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 :-)
(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.
(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
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
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
I would prefer isUserScalable, sounds more Qt'ish.
Created attachment 57704 [details] Patch Now using a struct: QWebPage::ViewportHints.
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.
Looking for more API feedback with https://lists.webkit.org/pipermail/webkit-qt/2010-June/000577.html before proceeding with the review.
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.
(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
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.
(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.
Created attachment 58190 [details] patch Addressed all previous comments.
+ , isUserScalable(-1.0) Isn't this supposed to be a bool?
Created attachment 58237 [details] patch
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.
Created attachment 58392 [details] patch Rebased and using {initial, minimum, maximum}ScaleFactor() instead of {initial, minimum, maximum}Scale().
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.
Created attachment 58393 [details] patch fixed include order.
(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.
(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?
(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.
Created attachment 58699 [details] patch Addressed the previous comments.
(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() :-)
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.
Created attachment 58922 [details] patch Added the missing members copying and assignments. :)
Comment on attachment 58922 [details] patch r=me
Committed r61342: <http://trac.webkit.org/changeset/61342>
Comment on attachment 58922 [details] patch Clearing cq flag.
Is this patch getting into QtWebkit 2.0?
Do you need it?
(In reply to comment #38) > Do you need it? Oh yes!
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.