| Summary: | Add common handling of viewport meta tag based on new Opera spec | ||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Kenneth Rohde Christiansen <kenneth> | ||||||||||||||||||||||||||||||||
| Component: | New Bugs | Assignee: | Kenneth Rohde Christiansen <kenneth> | ||||||||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||
| Severity: | Enhancement | CC: | ademar, annevk, christian.webkit, dbates, eric, gustavo, hausmann, joone, koivisto, luiz, mike, mrobinson, simon.fraser, tonikitoo, webkit-ews, webkit.review.bot | ||||||||||||||||||||||||||||||||
| Priority: | P3 | Keywords: | Qt | ||||||||||||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||||||
| Hardware: | All | ||||||||||||||||||||||||||||||||||
| OS: | All | ||||||||||||||||||||||||||||||||||
| URL: | http://people.opera.com/rune/TR/ED-css-viewport-20100806/ | ||||||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||||||
|
Description
Kenneth Rohde Christiansen
2010-08-18 13:42:55 PDT
Created attachment 64765 [details]
Patch (separate implementation with unittests)
Created attachment 64766 [details]
Patch (work in progress patch for WebCore)
Created attachment 64813 [details]
Patch 2 (webcore wip)
Created attachment 64839 [details]
Patch 3 (update Qt API)
Created attachment 65304 [details]
Patch 4 (with preliminary testing support)
Created attachment 65378 [details]
Patch 5 (with 50 tests)
Created attachment 65384 [details]
Patch 6 (with ~90 tests)
Parser test 65, 82, 84 and 87 are apparently failing according to the Opera result, so without expected result so far. Created attachment 65536 [details]
Patch 7 (incl all tests provided by Opera)
Created attachment 65747 [details]
Patch
Created attachment 65887 [details]
Patch (rebased)
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.
Attachment 65887 [details] did not build on mac: Build output: http://queues.webkit.org/results/3832130 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. (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. Created attachment 66884 [details]
Patch (rebased + warning fixes)
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.
Attachment 66884 [details] did not build on qt: Build output: http://queues.webkit.org/results/3953289 Attachment 66884 [details] did not build on mac: Build output: http://queues.webkit.org/results/3930273 Created attachment 66888 [details]
Patch (with fixes)
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.
Attachment 66888 [details] did not build on qt: Build output: http://queues.webkit.org/results/3942287 Created attachment 66892 [details]
Patch (yet another try)
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 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.
(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. Created attachment 66928 [details]
Patch (without non-ascii chars)
Ping review? 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?
> 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 on attachment 66928 [details]
Patch (without non-ascii chars)
Landed in 67376.
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 (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. Blocking our release bug as 2.1 already includes an API that is changed by this change. 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? (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. Revision r67376 cherry-picked into qtwebkit-2.1 with commit 0cf2d84b42da2333adefd8ba6586fd6652618010 |