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
: WebKit
New Bugs
: 528+ (Nightly build)
: All All
: P3 Enhancement
Assigned To:
: http://people.opera.com/rune/TR/ED-cs...
: Qt
:
:
  Show dependency treegraph
 
Reported: 2010-08-18 13:42 PST by
Modified: 2010-09-14 11:43 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-08-18 13:42:55 PST
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 From 2010-08-18 13:44:02 PST -------
Created an attachment (id=64765) [details]
Patch (separate implementation with unittests)
------- Comment #2 From 2010-08-18 13:44:50 PST -------
Created an attachment (id=64766) [details]
Patch (work in progress patch for WebCore)
------- Comment #3 From 2010-08-19 00:20:19 PST -------
Created an attachment (id=64813) [details]
Patch 2 (webcore wip)
------- Comment #4 From 2010-08-19 06:14:35 PST -------
Created an attachment (id=64839) [details]
Patch 3 (update Qt API)
------- Comment #5 From 2010-08-24 12:16:36 PST -------
Created an attachment (id=65304) [details]
Patch 4 (with preliminary testing support)
------- Comment #6 From 2010-08-24 23:51:57 PST -------
Created an attachment (id=65378) [details]
Patch 5 (with 50 tests)
------- Comment #7 From 2010-08-25 01:13:48 PST -------
Created an attachment (id=65384) [details]
Patch 6 (with ~90 tests)
------- Comment #8 From 2010-08-25 01:16:09 PST -------
Parser test 65, 82, 84 and 87 are apparently failing according to the Opera result, so without expected result so far.
------- Comment #9 From 2010-08-26 02:43:57 PST -------
Created an attachment (id=65536) [details]
Patch 7 (incl all tests provided by Opera)
------- Comment #10 From 2010-08-27 12:01:43 PST -------
Created an attachment (id=65747) [details]
Patch
------- Comment #11 From 2010-08-30 01:28:50 PST -------
Created an attachment (id=65887) [details]
Patch (rebased)
------- Comment #12 From 2010-08-30 01:39:31 PST -------
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 From 2010-08-30 01:54:40 PST -------
Attachment 65887 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3832130
------- Comment #14 From 2010-08-30 08:08:59 PST -------
(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.
------- Comment #15 From 2010-08-30 10:11:02 PST -------
(In reply to comment #14)
> (From update of attachment 65887 [details] [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 From 2010-09-08 05:28:23 PST -------
Created an attachment (id=66884) [details]
Patch (rebased + warning fixes)
------- Comment #17 From 2010-09-08 05:40:01 PST -------
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 From 2010-09-08 05:56:04 PST -------
Attachment 66884 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3953289
------- Comment #19 From 2010-09-08 06:00:59 PST -------
Attachment 66884 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3930273
------- Comment #20 From 2010-09-08 06:10:07 PST -------
Created an attachment (id=66888) [details]
Patch (with fixes)
------- Comment #21 From 2010-09-08 06:19:17 PST -------
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 From 2010-09-08 06:25:01 PST -------
Attachment 66888 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3942287
------- Comment #23 From 2010-09-08 07:01:16 PST -------
Created an attachment (id=66892) [details]
Patch (yet another try)
------- Comment #24 From 2010-09-08 12:17:39 PST -------
Created an attachment (id=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 From 2010-09-08 12:22:01 PST -------
(From update of attachment 66925 [details])
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 From 2010-09-08 12:26:03 PST -------
(In reply to comment #25)
> (From update of attachment 66925 [details] [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 From 2010-09-08 12:28:33 PST -------
Created an attachment (id=66928) [details]
Patch (without non-ascii chars)
------- Comment #28 From 2010-09-13 00:03:40 PST -------
Ping review?
------- Comment #29 From 2010-09-13 01:38:22 PST -------
(From update of attachment 66928 [details])
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 From 2010-09-13 01:43:48 PST -------
> 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 From 2010-09-13 03:39:44 PST -------
(From update of attachment 66928 [details])
Landed in 67376.
------- Comment #32 From 2010-09-13 04:49:30 PST -------
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 From 2010-09-13 05:52:52 PST -------
(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 From 2010-09-13 06:23:48 PST -------
Blocking our release bug as 2.1 already includes an API that is changed by this change.
------- Comment #35 From 2010-09-14 07:52:22 PST -------
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 From 2010-09-14 08:09:09 PST -------
(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 From 2010-09-14 11:43:23 PST -------
Revision r67376 cherry-picked into qtwebkit-2.1 with commit 0cf2d84b42da2333adefd8ba6586fd6652618010