Bug 39902 - [Qt] QtWebKit does not support viewport meta tag
Summary: [Qt] QtWebKit does not support viewport meta tag
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Jesus Sanchez-Palencia
URL:
Keywords: Qt
Depends on:
Blocks:
 
Reported: 2010-05-28 14:32 PDT by Jesus Sanchez-Palencia
Modified: 2010-07-26 02:00 PDT (History)
7 users (show)

See Also:


Attachments
patch (17.25 KB, patch)
2010-05-28 14:51 PDT, Jesus Sanchez-Palencia
no flags Details | Formatted Diff | Diff
patch (17.48 KB, patch)
2010-05-28 15:10 PDT, Jesus Sanchez-Palencia
no flags Details | Formatted Diff | Diff
patch (17.53 KB, patch)
2010-05-31 07:52 PDT, Jesus Sanchez-Palencia
no flags Details | Formatted Diff | Diff
patch (17.67 KB, patch)
2010-05-31 08:42 PDT, Jesus Sanchez-Palencia
no flags Details | Formatted Diff | Diff
Patch (19.91 KB, patch)
2010-06-02 14:18 PDT, Jesus Sanchez-Palencia
hausmann: review-
hausmann: commit-queue-
Details | Formatted Diff | Diff
patch (22.76 KB, patch)
2010-06-08 15:08 PDT, Jesus Sanchez-Palencia
no flags Details | Formatted Diff | Diff
patch (22.76 KB, patch)
2010-06-09 05:52 PDT, Jesus Sanchez-Palencia
no flags Details | Formatted Diff | Diff
patch (22.70 KB, patch)
2010-06-10 11:01 PDT, Jesus Sanchez-Palencia
no flags Details | Formatted Diff | Diff
patch (23.10 KB, patch)
2010-06-10 11:09 PDT, Jesus Sanchez-Palencia
no flags Details | Formatted Diff | Diff
patch (23.19 KB, patch)
2010-06-14 14:07 PDT, Jesus Sanchez-Palencia
hausmann: review-
hausmann: commit-queue-
Details | Formatted Diff | Diff
patch (23.73 KB, patch)
2010-06-16 13:39 PDT, Jesus Sanchez-Palencia
hausmann: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jesus Sanchez-Palencia 2010-05-28 14:32:28 PDT
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.
Comment 1 Jesus Sanchez-Palencia 2010-05-28 14:51:37 PDT
Created attachment 57386 [details]
patch
Comment 2 WebKit Review Bot 2010-05-28 14:56:10 PDT
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.
Comment 3 Jesus Sanchez-Palencia 2010-05-28 15:01:09 PDT
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
Comment 4 Jesus Sanchez-Palencia 2010-05-28 15:10:57 PDT
Created attachment 57387 [details]
patch
Comment 6 Simon Hausmann 2010-05-29 03:21:02 PDT
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 :(
Comment 7 Simon Hausmann 2010-05-29 11:11:26 PDT
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 :-)
Comment 8 Kenneth Rohde Christiansen 2010-05-29 11:16:32 PDT
(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.
Comment 9 Simon Hausmann 2010-05-29 14:35:42 PDT
(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
Comment 10 Jesus Sanchez-Palencia 2010-05-31 07:52:42 PDT
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
Comment 11 Jesus Sanchez-Palencia 2010-05-31 08:42:09 PDT
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
Comment 12 Kenneth Rohde Christiansen 2010-05-31 08:50:46 PDT
I would prefer isUserScalable, sounds more Qt'ish.
Comment 13 Jesus Sanchez-Palencia 2010-06-02 14:18:01 PDT
Created attachment 57704 [details]
Patch

Now using a struct: QWebPage::ViewportHints.
Comment 14 Simon Hausmann 2010-06-04 05:54:52 PDT
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.
Comment 15 Simon Hausmann 2010-06-04 06:03:08 PDT
Looking for more API feedback with https://lists.webkit.org/pipermail/webkit-qt/2010-June/000577.html before proceeding with the review.
Comment 16 Richard Moore 2010-06-04 14:37:42 PDT
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.
Comment 17 Kenneth Rohde Christiansen 2010-06-04 19:55:26 PDT
(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 18 Simon Hausmann 2010-06-07 07:49:12 PDT
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.
Comment 19 Jesus Sanchez-Palencia 2010-06-07 08:21:47 PDT
(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.
Comment 20 Jesus Sanchez-Palencia 2010-06-08 15:08:52 PDT
Created attachment 58190 [details]
patch

Addressed all previous comments.
Comment 21 Kenneth Rohde Christiansen 2010-06-09 05:39:23 PDT
+        , isUserScalable(-1.0)

Isn't this supposed to be a bool?
Comment 22 Jesus Sanchez-Palencia 2010-06-09 05:52:29 PDT
Created attachment 58237 [details]
patch
Comment 23 Ariya Hidayat 2010-06-10 09:41:43 PDT
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.
Comment 24 Jesus Sanchez-Palencia 2010-06-10 11:01:53 PDT
Created attachment 58392 [details]
patch

Rebased and using {initial, minimum, maximum}ScaleFactor() instead of {initial, minimum, maximum}Scale().
Comment 25 WebKit Review Bot 2010-06-10 11:02:48 PDT
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.
Comment 26 Jesus Sanchez-Palencia 2010-06-10 11:09:41 PDT
Created attachment 58393 [details]
patch

fixed include order.
Comment 27 Simon Hausmann 2010-06-10 22:50:20 PDT
(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.
Comment 28 Jesus Sanchez-Palencia 2010-06-11 04:29:20 PDT
(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?
Comment 29 Simon Hausmann 2010-06-14 07:28:39 PDT
(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.
Comment 30 Jesus Sanchez-Palencia 2010-06-14 14:07:52 PDT
Created attachment 58699 [details]
patch

Addressed the previous comments.
Comment 31 Simon Hausmann 2010-06-16 09:07:31 PDT
(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 32 Simon Hausmann 2010-06-16 09:09:55 PDT
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.
Comment 33 Jesus Sanchez-Palencia 2010-06-16 13:39:01 PDT
Created attachment 58922 [details]
patch

Added the missing members copying and assignments. :)
Comment 34 Simon Hausmann 2010-06-16 13:48:16 PDT
Comment on attachment 58922 [details]
patch

r=me
Comment 35 Jesus Sanchez-Palencia 2010-06-17 13:05:46 PDT
Committed r61342: <http://trac.webkit.org/changeset/61342>
Comment 36 Jesus Sanchez-Palencia 2010-06-17 13:07:37 PDT
Comment on attachment 58922 [details]
patch

Clearing cq flag.
Comment 37 Anand Patil 2010-06-21 23:12:22 PDT
Is this patch getting into QtWebkit 2.0?
Comment 38 Kenneth Rohde Christiansen 2010-06-22 06:42:21 PDT
Do you need it?
Comment 39 Anand Patil 2010-06-22 21:52:55 PDT
(In reply to comment #38)
> Do you need it?

Oh yes!
Comment 40 Simon Hausmann 2010-07-01 00:59:36 PDT
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.