Bug 44201 – Add common handling of viewport meta tag based on new Opera spec

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Kenneth Rohde Christiansen]

Add common handling of viewport meta tag based on the new Opera spec http://people.opera.com/rune/TR/ED-css-viewport-20100806/

Currently only the parsing of the viewport meta tag is in WebKit, resulting in many incompatible implementations of the actual handling of the data.

The idea is to move the processing algorithm into WebKit and test it with layout tests.

[Kenneth Rohde Christiansen]

Created attachment 64765 [details]
Patch (separate implementation with unittests)

[Kenneth Rohde Christiansen]

Created attachment 64766 [details]
Patch (work in progress patch for WebCore)

[Kenneth Rohde Christiansen]

Created attachment 64813 [details]
Patch 2 (webcore wip)

[Kenneth Rohde Christiansen]

Created attachment 64839 [details]
Patch 3 (update Qt API)

[Kenneth Rohde Christiansen]

Created attachment 65304 [details]
Patch 4 (with preliminary testing support)

[Kenneth Rohde Christiansen]

Created attachment 65378 [details]
Patch 5 (with 50 tests)

[Kenneth Rohde Christiansen]

Created attachment 65384 [details]
Patch 6 (with ~90 tests)

[Kenneth Rohde Christiansen]

Parser test 65, 82, 84 and 87 are apparently failing according to the Opera result, so without expected result so far.

[Kenneth Rohde Christiansen]

Created attachment 65536 [details]
Patch 7 (incl all tests provided by Opera)

[Kenneth Rohde Christiansen]

Created attachment 65747 [details]
Patch

[Kenneth Rohde Christiansen]

Created attachment 65887 [details]
Patch (rebased)

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebpage_p.h"
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebpage.cpp"
WebCore/dom/ViewportArguments.h:31:  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.h"
Total errors found: 1 in 248 files


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

[Eric Seidel (no email)]

Attachment 65887 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3832130

[Simon Hausmann]

Comment on attachment 65887 [details]
Patch (rebased)

> :2339
> +QWebPage::ViewportConfiguration QWebPage::findConfigurationForViewport(QSize availableSize) const
I would suggest to drop the term "find" from the method name and maybe just called it "viewportConfigurationForSize". In Qt we tend to use the term "find" when it's pretty clear that we're actually searching in a known set. Here it seems we're sort of calculating something.

[Kenneth Rohde Christiansen]

(In reply to comment #14)
> (From update of attachment 65887 [details])
> > :2339
> > +QWebPage::ViewportConfiguration QWebPage::findConfigurationForViewport(QSize availableSize) const
> I would suggest to drop the term "find" from the method name and maybe just called it "viewportConfigurationForSize". In Qt we tend to use the term "find" when it's pretty clear that we're actually searching in a known set. Here it seems we're sort of calculating something.

I wanted to show that the method is not cheap, just like when we use "toHtml()" instead of "html()" in our Qt APIs. I considered calc :-) but I don't like that very much.

[Kenneth Rohde Christiansen]

Created attachment 66884 [details]
Patch (rebased + warning fixes)

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebpage_p.h"
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebpage.cpp"
WebCore/dom/ViewportArguments.h:31:  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.h"
Total errors found: 1 in 248 files


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

[Early Warning System Bot]

Attachment 66884 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3953289

[Eric Seidel (no email)]

Attachment 66884 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3930273

[Kenneth Rohde Christiansen]

Created attachment 66888 [details]
Patch (with fixes)

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/dom/ViewportArguments.cpp:33:  Alphabetical sorting problem.  [build/include_order] [4]
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebpage_p.h"
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebpage.cpp"
WebCore/dom/ViewportArguments.h:31:  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.h"
Total errors found: 2 in 248 files


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

[Early Warning System Bot]

Attachment 66888 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3942287

[Kenneth Rohde Christiansen]

Created attachment 66892 [details]
Patch (yet another try)

[Kenneth Rohde Christiansen]

Created attachment 66925 [details]
Patch (with API change requested by Simon)

I'm not obsoleting the format patch yet because I want to make sure it passes on Windows first.

[Simon Fraser (smfr)]

Comment on attachment 66925 [details]
Patch (with API change requested by Simon)

That's a lot of tests. Can they be grouped together somehow?

 77     // Resolve non-âautoâ width and height to pixel values.
No non-ascii in source files please.

[Kenneth Rohde Christiansen]

(In reply to comment #25)
> (From update of attachment 66925 [details])
> That's a lot of tests. Can they be grouped together somehow?

Not really as they depend on one meta tag per test, and if we group them we would have to find a logical way of doing so, and if they fail we would manually have to find out why. Having each test separate makes that easier.

>  77     // Resolve non-âautoâ width and height to pixel values.
> No non-ascii in source files please.

I will fix that.

[Kenneth Rohde Christiansen]

Created attachment 66928 [details]
Patch (without non-ascii chars)

[Kenneth Rohde Christiansen]

Ping review?

[Antti Koivisto]

Comment on attachment 66928 [details]
Patch (without non-ascii chars)

Looks good and well tested so r=me

It might be good to explain in comments where the magic constants come from (the spec).

It is not entirely clear to me why the code dealing with available sizes can't result in a division by zero as there is no obvious zero checking:

width = args.height * (availableWidth / availableHeight);
height = width * availableHeight / availableWidth;

Could you still double-check the logic?

[Kenneth Rohde Christiansen]

> It is not entirely clear to me why the code dealing with available sizes can't result in a division by zero as there is no obvious zero checking:
> 
> width = args.height * (availableWidth / availableHeight);
> height = width * availableHeight / availableWidth;
> 
> Could you still double-check the logic?

If neither availableWidth or availableHeight is positive, we have nothing to show. We should add an assert for availableWidth > 0 && availableHeight > 0.

[Kenneth Rohde Christiansen]

Comment on attachment 66928 [details]
Patch (without non-ascii chars)

Landed in 67376.

[Antti Koivisto]

podivilov: hey, looks like http://trac.webkit.org/changeset/67376 doesn't have expectations for
[14:47] podivilov: fast/viewport/viewport-126.html = MISSING
[14:47] podivilov: fast/viewport/viewport-127.html = MISSING
[14:47] podivilov: fast/viewport/viewport-65.html = MISSING
[14:47] podivilov: fast/viewport/viewport-82.html = MISSING
[14:47] podivilov: fast/viewport/viewport-84.html = MISSING
[14:47] podivilov: fast/viewport/viewport-87.html = MISSING

[Kenneth Rohde Christiansen]

(In reply to comment #32)
> podivilov: hey, looks like http://trac.webkit.org/changeset/67376 doesn't have expectations for
> [14:47] podivilov: fast/viewport/viewport-126.html = MISSING
> [14:47] podivilov: fast/viewport/viewport-127.html = MISSING
> [14:47] podivilov: fast/viewport/viewport-65.html = MISSING
> [14:47] podivilov: fast/viewport/viewport-82.html = MISSING
> [14:47] podivilov: fast/viewport/viewport-84.html = MISSING
> [14:47] podivilov: fast/viewport/viewport-87.html = MISSING

That is indeed correct. We skipped these for Qt, as their result is not equal to that observed by Opera on the iPhone. I'm sorry for not doing the chromium results, but I was confused about how exactly to skip the tests.

[Kenneth Rohde Christiansen]

Blocking our release bug as 2.1 already includes an API that is changed by this change.

[Ademar Reis]

About cherry-picking it to qtwebkit-2.1:

The patch touches parts of the code changed due to Bug 43492 (Android target-densitydpi extension). I have the impression we should include both patches, but Bug 43492 is not marked for inclusion in QtWebkit-2.1.

Could you please confirm that?

[Kenneth Rohde Christiansen]

(In reply to comment #35)
> About cherry-picking it to qtwebkit-2.1:
> 
> The patch touches parts of the code changed due to Bug 43492 (Android target-densitydpi extension). I have the impression we should include both patches, but Bug 43492 is not marked for inclusion in QtWebkit-2.1.
> 
> Could you please confirm that?

You are right, you need to include both patches. sorry about that.

[Ademar Reis]

Revision r67376 cherry-picked into qtwebkit-2.1 with commit 0cf2d84b42da2333adefd8ba6586fd6652618010