Bug 44201 - Add common handling of viewport meta tag based on new Opera spec
: Add common handling of viewport meta tag based on new Opera spec
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: New Bugs
: 528+ (Nightly build)
: All All
: P3 Enhancement
Assigned To: Kenneth Rohde Christiansen
http://people.opera.com/rune/TR/ED-cs...
: Qt
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-08-18 13:42 PDT by Kenneth Rohde Christiansen
Modified: 2010-09-14 11:43 PDT (History)
16 users (show)

See Also:


Attachments
Patch (separate implementation with unittests) (14.90 KB, patch)
2010-08-18 13:44 PDT, Kenneth Rohde Christiansen
no flags Details | Formatted Diff | Diff
Patch (work in progress patch for WebCore) (16.18 KB, patch)
2010-08-18 13:44 PDT, Kenneth Rohde Christiansen
no flags Details | Formatted Diff | Diff
Patch 2 (webcore wip) (16.63 KB, patch)
2010-08-19 00:20 PDT, Kenneth Rohde Christiansen
no flags Details | Formatted Diff | Diff
Patch 3 (update Qt API) (31.88 KB, patch)
2010-08-19 06:14 PDT, Kenneth Rohde Christiansen
no flags Details | Formatted Diff | Diff
Patch 4 (with preliminary testing support) (39.03 KB, patch)
2010-08-24 12:16 PDT, Kenneth Rohde Christiansen
no flags Details | Formatted Diff | Diff
Patch 5 (with 50 tests) (95.99 KB, patch)
2010-08-24 23:51 PDT, Kenneth Rohde Christiansen
no flags Details | Formatted Diff | Diff
Patch 6 (with ~90 tests) (141.65 KB, patch)
2010-08-25 01:13 PDT, Kenneth Rohde Christiansen
no flags Details | Formatted Diff | Diff
Patch 7 (incl all tests provided by Opera) (174.44 KB, patch)
2010-08-26 02:43 PDT, Kenneth Rohde Christiansen
no flags Details | Formatted Diff | Diff
Patch (215.98 KB, patch)
2010-08-27 12:01 PDT, Kenneth Rohde Christiansen
no flags Details | Formatted Diff | Diff
Patch (rebased) (216.12 KB, patch)
2010-08-30 01:28 PDT, Kenneth Rohde Christiansen
no flags Details | Formatted Diff | Diff
Patch (rebased + warning fixes) (215.97 KB, patch)
2010-09-08 05:28 PDT, Kenneth Rohde Christiansen
no flags Details | Formatted Diff | Diff
Patch (with fixes) (216.01 KB, patch)
2010-09-08 06:10 PDT, Kenneth Rohde Christiansen
no flags Details | Formatted Diff | Diff
Patch (yet another try) (215.97 KB, patch)
2010-09-08 07:01 PDT, Kenneth Rohde Christiansen
no flags Details | Formatted Diff | Diff
Patch (with API change requested by Simon) (215.97 KB, patch)
2010-09-08 12:17 PDT, Kenneth Rohde Christiansen
no flags Details | Formatted Diff | Diff
Patch (without non-ascii chars) (215.97 KB, patch)
2010-09-08 12:28 PDT, Kenneth Rohde Christiansen
koivisto: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kenneth Rohde Christiansen 2010-08-18 13:42:55 PDT
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.
Comment 1 Kenneth Rohde Christiansen 2010-08-18 13:44:02 PDT
Created attachment 64765 [details]
Patch (separate implementation with unittests)
Comment 2 Kenneth Rohde Christiansen 2010-08-18 13:44:50 PDT
Created attachment 64766 [details]
Patch (work in progress patch for WebCore)
Comment 3 Kenneth Rohde Christiansen 2010-08-19 00:20:19 PDT
Created attachment 64813 [details]
Patch 2 (webcore wip)
Comment 4 Kenneth Rohde Christiansen 2010-08-19 06:14:35 PDT
Created attachment 64839 [details]
Patch 3 (update Qt API)
Comment 5 Kenneth Rohde Christiansen 2010-08-24 12:16:36 PDT
Created attachment 65304 [details]
Patch 4 (with preliminary testing support)
Comment 6 Kenneth Rohde Christiansen 2010-08-24 23:51:57 PDT
Created attachment 65378 [details]
Patch 5 (with 50 tests)
Comment 7 Kenneth Rohde Christiansen 2010-08-25 01:13:48 PDT
Created attachment 65384 [details]
Patch 6 (with ~90 tests)
Comment 8 Kenneth Rohde Christiansen 2010-08-25 01:16:09 PDT
Parser test 65, 82, 84 and 87 are apparently failing according to the Opera result, so without expected result so far.
Comment 9 Kenneth Rohde Christiansen 2010-08-26 02:43:57 PDT
Created attachment 65536 [details]
Patch 7 (incl all tests provided by Opera)
Comment 10 Kenneth Rohde Christiansen 2010-08-27 12:01:43 PDT
Created attachment 65747 [details]
Patch
Comment 11 Kenneth Rohde Christiansen 2010-08-30 01:28:50 PDT
Created attachment 65887 [details]
Patch (rebased)
Comment 12 WebKit Review Bot 2010-08-30 01:39:31 PDT
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.
Comment 13 Eric Seidel 2010-08-30 01:54:40 PDT
Attachment 65887 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3832130
Comment 14 Simon Hausmann 2010-08-30 08:08:59 PDT
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.
Comment 15 Kenneth Rohde Christiansen 2010-08-30 10:11:02 PDT
(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.
Comment 16 Kenneth Rohde Christiansen 2010-09-08 05:28:23 PDT
Created attachment 66884 [details]
Patch (rebased + warning fixes)
Comment 17 WebKit Review Bot 2010-09-08 05:40:01 PDT
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.
Comment 18 Early Warning System Bot 2010-09-08 05:56:04 PDT
Attachment 66884 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3953289
Comment 19 Eric Seidel 2010-09-08 06:00:59 PDT
Attachment 66884 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3930273
Comment 20 Kenneth Rohde Christiansen 2010-09-08 06:10:07 PDT
Created attachment 66888 [details]
Patch (with fixes)
Comment 21 WebKit Review Bot 2010-09-08 06:19:17 PDT
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.
Comment 22 Early Warning System Bot 2010-09-08 06:25:01 PDT
Attachment 66888 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3942287
Comment 23 Kenneth Rohde Christiansen 2010-09-08 07:01:16 PDT
Created attachment 66892 [details]
Patch (yet another try)
Comment 24 Kenneth Rohde Christiansen 2010-09-08 12:17:39 PDT
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.
Comment 25 Simon Fraser (smfr) 2010-09-08 12:22:01 PDT
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.
Comment 26 Kenneth Rohde Christiansen 2010-09-08 12:26:03 PDT
(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.
Comment 27 Kenneth Rohde Christiansen 2010-09-08 12:28:33 PDT
Created attachment 66928 [details]
Patch (without non-ascii chars)
Comment 28 Kenneth Rohde Christiansen 2010-09-13 00:03:40 PDT
Ping review?
Comment 29 Antti Koivisto 2010-09-13 01:38:22 PDT
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?
Comment 30 Kenneth Rohde Christiansen 2010-09-13 01:43:48 PDT
> 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.
Comment 31 Kenneth Rohde Christiansen 2010-09-13 03:39:44 PDT
Comment on attachment 66928 [details]
Patch (without non-ascii chars)

Landed in 67376.
Comment 32 Antti Koivisto 2010-09-13 04:49:30 PDT
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
Comment 33 Kenneth Rohde Christiansen 2010-09-13 05:52:52 PDT
(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.
Comment 34 Kenneth Rohde Christiansen 2010-09-13 06:23:48 PDT
Blocking our release bug as 2.1 already includes an API that is changed by this change.
Comment 35 Ademar Reis 2010-09-14 07:52:22 PDT
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?
Comment 36 Kenneth Rohde Christiansen 2010-09-14 08:09:09 PDT
(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.
Comment 37 Ademar Reis 2010-09-14 11:43:23 PDT
Revision r67376 cherry-picked into qtwebkit-2.1 with commit 0cf2d84b42da2333adefd8ba6586fd6652618010