Bug 70609

Summary: Removing line in computeViewportAttributes that enforces a minimum scale factor to never allow zooming out more than viewport
Product: WebKit Reporter: Fady Samuel <fsamuel>
Component: Layout and RenderingAssignee: Fady Samuel <fsamuel>
Status: RESOLVED FIXED    
Severity: Normal CC: abecsi, gustavo, johnme, kenneth, kpiascik, menard, mrobinson, ossy, peter, pnormand, rakuco, rjkroege, tonikitoo, webkit.review.bot, xan.lopez, zalan, zoltan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 85458    
Bug Blocks: 70559    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
fsamuel: commit-queue+
Patch for landing
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Fady Samuel
Reported 2011-10-21 08:09:01 PDT
The body may be larger than the viewport and so we'd never be able to zoom out fully. Please refer to: http://www.w3.org/TR/2011/WD-css-device-adapt-20110915/#constraining-viewport-property-values
Attachments
Patch (3.18 KB, patch)
2011-10-25 12:15 PDT, Fady Samuel
no flags
Patch (13.60 KB, patch)
2011-11-01 09:13 PDT, Fady Samuel
no flags
Patch (13.83 KB, patch)
2011-11-01 10:19 PDT, Fady Samuel
no flags
Patch (14.67 KB, patch)
2011-11-02 10:52 PDT, Fady Samuel
fsamuel: commit-queue+
Patch for landing (10.19 KB, patch)
2011-11-03 07:30 PDT, Fady Samuel
no flags
Patch (14.61 KB, patch)
2011-11-03 08:15 PDT, Fady Samuel
no flags
Patch (14.67 KB, patch)
2011-11-03 08:19 PDT, Fady Samuel
no flags
Patch (2.41 KB, patch)
2012-04-24 11:04 PDT, Fady Samuel
no flags
Patch (3.03 KB, patch)
2012-05-02 08:12 PDT, Fady Samuel
no flags
Patch (4.38 KB, patch)
2012-05-02 08:31 PDT, Fady Samuel
no flags
Patch (5.28 KB, patch)
2012-05-02 09:36 PDT, Fady Samuel
no flags
Patch (7.72 KB, patch)
2012-05-02 10:06 PDT, Fady Samuel
no flags
Patch (7.72 KB, patch)
2012-05-02 11:20 PDT, Fady Samuel
no flags
Patch (7.78 KB, patch)
2012-05-02 12:09 PDT, Fady Samuel
no flags
Patch (9.04 KB, patch)
2012-05-02 13:42 PDT, Fady Samuel
no flags
Patch (15.62 KB, patch)
2012-05-03 08:17 PDT, Fady Samuel
no flags
Patch (15.45 KB, patch)
2012-05-03 08:28 PDT, Fady Samuel
no flags
Patch (17.27 KB, patch)
2012-05-03 10:23 PDT, Fady Samuel
no flags
Patch (19.03 KB, patch)
2012-05-03 10:46 PDT, Fady Samuel
no flags
Fady Samuel
Comment 1 2011-10-21 08:12:18 PDT
For reference please see the file Source/WebCore/dom/ViewportArguments.cpp: http://www.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/dom/ViewportArguments.cpp&exact_package=chromium&q=ViewportArguments.cpp&type=cs We would like to delete these lines or at least put them behind a (default) parameter: // Update minimum scale factor, to never allow zooming out more than viewport result.minimumScale = max<float>(result.minimumScale, max(availableWidth / width, availableHeight / height));
Fady Samuel
Comment 2 2011-10-25 12:15:29 PDT
Kenneth Rohde Christiansen
Comment 3 2011-10-25 15:32:43 PDT
So basically what you want is the support for fit
Fady Samuel
Comment 4 2011-10-25 15:36:44 PDT
(In reply to comment #3) > So basically what you want is the support for fit Yup. We want to let the embedder decide on the minimum scale factor.
Kenneth Rohde Christiansen
Comment 5 2011-10-25 15:37:34 PDT
So this is tricky area. The content is larger than the viewport and you cannot zoom fully out. That might very well be how the web page was designed so as a general rule we try to not work around bad content. What is really important here is whether the scale restriction was explicitly set or implicitly calculated. Zalan (which I just cc'ed) has some patches for dealing with that. Zalan, maybe you can chime in?
Daniel Bates
Comment 6 2011-10-25 16:39:53 PDT
Fady Samuel
Comment 7 2011-10-26 08:42:27 PDT
(In reply to comment #5) > So this is tricky area. The content is larger than the viewport and you cannot zoom fully out. > > That might very well be how the web page was designed so as a general rule we try to not work around bad content. > > What is really important here is whether the scale restriction was explicitly set or implicitly calculated. > > Zalan (which I just cc'ed) has some patches for dealing with that. Zalan, maybe you can chime in? It depends on what you consider the purpose of overview mode to be? If I'm in overview mode I'd like to be able to look at the width of the page at a glance, without scrolling. The change I'm proposing leaves this decision in the hands of the embedder. Existing ports that make use of this function will not need to make any changes, because, by default, nothing has changed.
Kenneth Rohde Christiansen
Comment 8 2011-10-26 09:24:30 PDT
(In reply to comment #7) > (In reply to comment #5) > > So this is tricky area. The content is larger than the viewport and you cannot zoom fully out. > > > > That might very well be how the web page was designed so as a general rule we try to not work around bad content. > > > > What is really important here is whether the scale restriction was explicitly set or implicitly calculated. > > > > Zalan (which I just cc'ed) has some patches for dealing with that. Zalan, maybe you can chime in? > > It depends on what you consider the purpose of overview mode to be? If I'm in overview mode I'd like to be able to look at the width of the page at a glance, without scrolling. The change I'm proposing leaves this decision in the hands of the embedder. Existing ports that make use of this function will not need to make any changes, because, by default, nothing has changed. You don't need this change for that. Your embedder can just ignore the proposed scale values and scale such that the full width is visible. You might have to update your layout height then, by multiplying the width with the ratio you want for your overview thumbnails.
zalan
Comment 9 2011-10-26 09:34:57 PDT
we, with n9 browser, do pretty much the same by ignoring implicit minimum values for certain cases. The logic is at the app level and because we decided not to force such parameters through the viewport API.
Fady Samuel
Comment 10 2011-10-26 09:42:58 PDT
(In reply to comment #9) > we, with n9 browser, do pretty much the same by ignoring implicit minimum values for certain cases. The logic is at the app level and because we decided not to force such parameters through the viewport API. We are in agreement! It seems like the best solution is to get this code into WebCore if multiple ports are interested in the same behavior.
Kenneth Rohde Christiansen
Comment 11 2011-10-26 09:48:30 PDT
(In reply to comment #10) > (In reply to comment #9) > > we, with n9 browser, do pretty much the same by ignoring implicit minimum values for certain cases. The logic is at the app level and because we decided not to force such parameters through the viewport API. > > We are in agreement! It seems like the best solution is to get this code into WebCore if multiple ports are interested in the same behavior. I totally disagree. The code in WebCore should be as simple as possible and just do the calculations in accordance with the spec. Fit to width, overview thumbnails are all user-agent/browser things on top and they should therefore be done by the user-agent and not intertwined in the core algorithm.
Kenneth Rohde Christiansen
Comment 12 2011-10-26 09:50:13 PDT
Comment on attachment 112373 [details] Patch Let's not open for a can of worms. The code in WebCore is for the basic algorithm and should stay as such. If different behavior is wished for, it can easily be done on top.
zalan
Comment 13 2011-10-26 09:54:38 PDT
(In reply to comment #12) > (From update of attachment 112373 [details]) > Let's not open for a can of worms. The code in WebCore is for the basic algorithm and should stay as such. If different behavior is wished for, it can easily be done on top. Agree. That's why we decided to do it at the app level. We also have some specific adjustment for initial and maximum values. Pushing all of them through the API would make it end up looking really broken.
John Mellor
Comment 14 2011-10-27 08:01:23 PDT
(In reply to comment #11) > I totally disagree. The code in WebCore should be as simple as possible and just do the calculations in accordance with the spec. Fit to width, overview thumbnails are all user-agent/browser things on top and they should therefore be done by the user-agent and not intertwined in the core algorithm. We all agree that WebCore should just be a simple implementation of the spec. However if you compare the spec (http://www.w3.org/TR/2011/WD-css-device-adapt-20110915/#constraining-viewport-property-values) to the algorithm implemented in computeViewportAttributes, you will see that they roughly match up, until the end of "Extend width and height to fill the window/viewing area for the specified zoom". Then computeViewportAttributes does two things which are _not_ in the spec: 1. It modifies the minScale to avoid "zooming out more than viewport". 2. It overwrites min/maxScale if userScalable is false. Both of these additions are counter to the spec. While the 2nd one is slighty less of a problem, the 1st is an implementation-specific heuristic, making the -- often wrong -- assumption that embedders will never want to zoom out more than the viewport. And if the embedder does want to zoom out more than the viewport (as at least we and N9 do), it is no longer possible! Since that line has overwritten the true value of minScale, the embedder is unable to distinguish whether the webpage had explictly forbidden zooming out more than the viewport (in which case that should be respected) or if minScale was small or auto (in which case we can zoom out to show the whole page). We should remove this line from computeViewportAttributes, and instead any embedders which chose to have this implementation-specific behavior should do it themselves.
Kenneth Rohde Christiansen
Comment 15 2011-11-01 07:14:56 PDT
(In reply to comment #14) > (In reply to comment #11) > > I totally disagree. The code in WebCore should be as simple as possible and just do the calculations in accordance with the spec. Fit to width, overview thumbnails are all user-agent/browser things on top and they should therefore be done by the user-agent and not intertwined in the core algorithm. > > We all agree that WebCore should just be a simple implementation of the spec. However if you compare the spec (http://www.w3.org/TR/2011/WD-css-device-adapt-20110915/#constraining-viewport-property-values) to the algorithm implemented in computeViewportAttributes, you will see that they roughly match up, until the end of "Extend width and height to fill the window/viewing area for the specified zoom". Then computeViewportAttributes does two things which are _not_ in the spec: > > 1. It modifies the minScale to avoid "zooming out more than viewport". > 2. It overwrites min/maxScale if userScalable is false. > > Both of these additions are counter to the spec. While the 2nd one is slighty less of a problem, the 1st is an implementation-specific heuristic, making the -- often wrong -- assumption that embedders will never want to zoom out more than the viewport. > > And if the embedder does want to zoom out more than the viewport (as at least we and N9 do), it is no longer possible! Since that line has overwritten the true value of minScale, the embedder is unable to distinguish whether the webpage had explictly forbidden zooming out more than the viewport (in which case that should be respected) or if minScale was small or auto (in which case we can zoom out to show the whole page). > > We should remove this line from computeViewportAttributes, and instead any embedders which chose to have this implementation-specific behavior should do it themselves. I see. I guess we should just remove the line then and add this to the client. That is fine with me.
Fady Samuel
Comment 16 2011-11-01 09:13:47 PDT
Fady Samuel
Comment 17 2011-11-01 09:15:01 PDT
(In reply to comment #15) > (In reply to comment #14) > > (In reply to comment #11) > > > I totally disagree. The code in WebCore should be as simple as possible and just do the calculations in accordance with the spec. Fit to width, overview thumbnails are all user-agent/browser things on top and they should therefore be done by the user-agent and not intertwined in the core algorithm. > > > > We all agree that WebCore should just be a simple implementation of the spec. However if you compare the spec (http://www.w3.org/TR/2011/WD-css-device-adapt-20110915/#constraining-viewport-property-values) to the algorithm implemented in computeViewportAttributes, you will see that they roughly match up, until the end of "Extend width and height to fill the window/viewing area for the specified zoom". Then computeViewportAttributes does two things which are _not_ in the spec: > > > > 1. It modifies the minScale to avoid "zooming out more than viewport". > > 2. It overwrites min/maxScale if userScalable is false. > > > > Both of these additions are counter to the spec. While the 2nd one is slighty less of a problem, the 1st is an implementation-specific heuristic, making the -- often wrong -- assumption that embedders will never want to zoom out more than the viewport. > > > > And if the embedder does want to zoom out more than the viewport (as at least we and N9 do), it is no longer possible! Since that line has overwritten the true value of minScale, the embedder is unable to distinguish whether the webpage had explictly forbidden zooming out more than the viewport (in which case that should be respected) or if minScale was small or auto (in which case we can zoom out to show the whole page). > > > > We should remove this line from computeViewportAttributes, and instead any embedders which chose to have this implementation-specific behavior should do it themselves. > > I see. I guess we should just remove the line then and add this to the client. That is fine with me. I've uploaded a new version of the patch, deleting that line from computeViewportAttributes and I've updated all the clients that use it. It'll probably take me a couple of attempts to get all the bots to turn green.
Konrad Piascik
Comment 18 2011-11-01 09:29:09 PDT
You're only updating minimum scale, but what if the viewport arguments specified an initial scale that is smaller than your clamped minimum scale? ie: initial-scale:0.9 and your minimum-scale is restricted to 1.0 by the visibleViewport?
Fady Samuel
Comment 19 2011-11-01 09:32:16 PDT
(In reply to comment #18) > You're only updating minimum scale, but what if the viewport arguments specified an initial scale that is smaller than your clamped minimum scale? > ie: initial-scale:0.9 and your minimum-scale is restricted to 1.0 by the visibleViewport? Good catch! This would happen if !args.userScalable in addition to the above. Maybe I should move out these lines out to a different method too: if (!args.userScalable) result.maximumScale = result.minimumScale = result.initialScale;
Early Warning System Bot
Comment 20 2011-11-01 09:32:52 PDT
Gustavo Noronha (kov)
Comment 21 2011-11-01 09:33:59 PDT
Fady Samuel
Comment 22 2011-11-01 09:37:32 PDT
(In reply to comment #19) > (In reply to comment #18) > > You're only updating minimum scale, but what if the viewport arguments specified an initial scale that is smaller than your clamped minimum scale? > > ie: initial-scale:0.9 and your minimum-scale is restricted to 1.0 by the visibleViewport? > > > > Good catch! This would happen if !args.userScalable in addition to the above. Maybe I should move out these lines out to a different method too: > > > if (!args.userScalable) > result.maximumScale = result.minimumScale = result.initialScale; Adding method: restrictScaleFactorToInitialScaleIfNotUserScalable
Fady Samuel
Comment 23 2011-11-01 10:19:00 PDT
Daniel Bates
Comment 24 2011-11-01 15:24:03 PDT
Daniel Bates
Comment 25 2011-11-01 17:37:05 PDT
Kenneth Rohde Christiansen
Comment 26 2011-11-02 01:56:33 PDT
Comment on attachment 113186 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=113186&action=review Look pretty good. Zalan what do you think? > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2616 > ViewportAttributes attrs = WebCore::computeViewportAttributes(arguments, /* default layout width for non-mobile pages */ 980, deviceWidth, deviceHeight, deviceDPI, IntSize(availableWidth, availableHeight)); > + WebCore::restrictMinimumScaleFactorToViewportSize(attrs, IntSize(availableWidth, availableHeight)); > + WebCore::restrictScaleFactorToInitialScaleIfNotUserScalable(attrs); > return String::format("viewport size %dx%d scale %f with limits [%f, %f] and userScalable %f\n", attrs.layoutSize.width(), attrs.layoutSize.height(), attrs.initialScale, attrs.minimumScale, attrs.maximumScale, attrs.userScalable); I think it might be better to update the test results. The layout tests should test the implementation of the spec and the compliance.
zalan
Comment 27 2011-11-02 03:32:12 PDT
(In reply to comment #26) > (From update of attachment 113186 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=113186&action=review > > Look pretty good. Zalan what do you think? Looks pretty good to me too.
Kenneth Rohde Christiansen
Comment 28 2011-11-02 05:09:05 PDT
Comment on attachment 113186 [details] Patch Please make it work on mac become committing
Fady Samuel
Comment 29 2011-11-02 10:52:58 PDT
Fady Samuel
Comment 30 2011-11-02 11:29:11 PDT
(In reply to comment #28) > (From update of attachment 113186 [details]) > Please make it work on mac become committing Waiting on mac bot to turn green before committing.
Fady Samuel
Comment 31 2011-11-03 07:30:23 PDT
Created attachment 113487 [details] Patch for landing
Fady Samuel
Comment 32 2011-11-03 07:50:27 PDT
(In reply to comment #31) > Created an attachment (id=113487) [details] > Patch for landing Oops, accidentally lost the ChangeLog, I just removed the c+ flag. Fixing this then uploading a new patch and landing it.
Fady Samuel
Comment 33 2011-11-03 08:15:45 PDT
Fady Samuel
Comment 34 2011-11-03 08:19:21 PDT
Fady Samuel
Comment 35 2011-11-03 08:22:25 PDT
Comment on attachment 113495 [details] Patch Clearing flags on attachment: 113495 Committed r99195: <http://trac.webkit.org/changeset/99195>
Fady Samuel
Comment 36 2011-11-03 08:22:34 PDT
All reviewed patches have been landed. Closing bug.
Csaba Osztrogonác
Comment 37 2011-11-03 08:54:33 PDT
(In reply to comment #36) > All reviewed patches have been landed. Closing bug. It broke the Qt-WK2 build: /ramdisk/qt-linux-32-release-webkit2/build/Source/WebKit2/UIProcess/API/qt/qtouchwebview.cpp: In member function ‘void QTouchWebViewPrivate::updateViewportConstraints()’: /ramdisk/qt-linux-32-release-webkit2/build/Source/WebKit2/UIProcess/API/qt/qtouchwebview.cpp:95: error: ‘restrictMinimumScaleFactorToViewport’ is not a member of ‘WebCore’
Fady Samuel
Comment 38 2011-11-03 08:57:26 PDT
(In reply to comment #37) > (In reply to comment #36) > > All reviewed patches have been landed. Closing bug. > > It broke the Qt-WK2 build: > > /ramdisk/qt-linux-32-release-webkit2/build/Source/WebKit2/UIProcess/API/qt/qtouchwebview.cpp: In member function ‘void QTouchWebViewPrivate::updateViewportConstraints()’: > /ramdisk/qt-linux-32-release-webkit2/build/Source/WebKit2/UIProcess/API/qt/qtouchwebview.cpp:95: error: ‘restrictMinimumScaleFactorToViewport’ is not a member of ‘WebCore’ Ohh, that's a typo That should say restrictMinimumScaleFactorToViewportSize. I'm surprised the QT bot was green.
Csaba Osztrogonác
Comment 39 2011-11-03 08:59:18 PDT
(In reply to comment #38) > (In reply to comment #37) > > (In reply to comment #36) > > > All reviewed patches have been landed. Closing bug. > > > > It broke the Qt-WK2 build: > > > > /ramdisk/qt-linux-32-release-webkit2/build/Source/WebKit2/UIProcess/API/qt/qtouchwebview.cpp: In member function ‘void QTouchWebViewPrivate::updateViewportConstraints()’: > > /ramdisk/qt-linux-32-release-webkit2/build/Source/WebKit2/UIProcess/API/qt/qtouchwebview.cpp:95: error: ‘restrictMinimumScaleFactorToViewport’ is not a member of ‘WebCore’ > Ohh, that's a typo That should say restrictMinimumScaleFactorToViewportSize. I'm surprised the QT bot was green. Because Qt bot on build.webkit.org doesn't build WK2.
Csaba Osztrogonác
Comment 40 2011-11-03 09:27:31 PDT
Csaba Osztrogonác
Comment 41 2011-11-03 09:52:19 PDT
Qt-WK2 buildfix landed in http://trac.webkit.org/changeset/99199
Csaba Osztrogonác
Comment 42 2011-11-03 10:29:25 PDT
I added failing tests to the Skipped list until fix to make buildbot green to be able catch new regressions. Please unskip them with a fix. http://trac.webkit.org/changeset/99213/trunk/LayoutTests/platform/qt/Skipped
Fady Samuel
Comment 43 2011-11-03 10:43:55 PDT
(In reply to comment #42) > I added failing tests to the Skipped list until fix to make buildbot green to be able catch new regressions. Please unskip them with a fix. > > http://trac.webkit.org/changeset/99213/trunk/LayoutTests/platform/qt/Skipped I looked at the difference in output briefly, and it seems very very small. It's possible this is a small rounding issue but I haven't verified. I will be doing Chromium-side testing of all this shortly.
Csaba Osztrogonác
Comment 44 2011-11-03 11:21:49 PDT
These tests fail on GTK too.
Philippe Normand
Comment 45 2011-11-04 02:04:50 PDT
(In reply to comment #44) > These tests fail on GTK too. Skipped in GTK too: http://trac.webkit.org/changeset/99269 Please unskip when fixed!
Csaba Osztrogonác
Comment 46 2012-01-24 08:09:23 PST
This regression is still valid. Are you guys going to fix it?
Fady Samuel
Comment 47 2012-01-24 08:13:19 PST
(In reply to comment #46) > This regression is still valid. Are you guys going to fix it? I will look into this.
Philippe Normand
Comment 48 2012-03-12 08:16:40 PDT
(In reply to comment #47) > (In reply to comment #46) > > This regression is still valid. Are you guys going to fix it? > > I will look into this. Any news on this issue?
Csaba Osztrogonác
Comment 49 2012-03-23 07:47:34 PDT
It is a 4.5 month old regression ... It isn't a fairplay to leave it unresolved.
Fady Samuel
Comment 50 2012-04-24 11:04:44 PDT
Fady Samuel
Comment 51 2012-04-24 11:11:58 PDT
(In reply to comment #50) > Created an attachment (id=138600) [details] > Patch (In reply to comment #49) > It is a 4.5 month old regression ... It isn't a fairplay to leave it unresolved. Sorry, I was a little inactive on WebKit the last few months.
Build Bot
Comment 52 2012-04-24 11:30:52 PDT
Early Warning System Bot
Comment 53 2012-04-24 12:23:23 PDT
Early Warning System Bot
Comment 54 2012-04-24 13:36:34 PDT
Fady Samuel
Comment 55 2012-05-02 08:12:06 PDT
Fady Samuel
Comment 56 2012-05-02 08:31:09 PDT
Early Warning System Bot
Comment 57 2012-05-02 09:08:28 PDT
Early Warning System Bot
Comment 58 2012-05-02 09:16:11 PDT
Fady Samuel
Comment 59 2012-05-02 09:36:51 PDT
Early Warning System Bot
Comment 60 2012-05-02 09:57:16 PDT
Early Warning System Bot
Comment 61 2012-05-02 09:59:31 PDT
Fady Samuel
Comment 62 2012-05-02 10:06:06 PDT
Early Warning System Bot
Comment 63 2012-05-02 11:02:19 PDT
Early Warning System Bot
Comment 64 2012-05-02 11:03:24 PDT
Fady Samuel
Comment 65 2012-05-02 11:20:13 PDT
Early Warning System Bot
Comment 66 2012-05-02 12:01:16 PDT
Early Warning System Bot
Comment 67 2012-05-02 12:04:30 PDT
Fady Samuel
Comment 68 2012-05-02 12:09:56 PDT
Early Warning System Bot
Comment 69 2012-05-02 13:18:23 PDT
Early Warning System Bot
Comment 70 2012-05-02 13:35:41 PDT
Fady Samuel
Comment 71 2012-05-02 13:42:17 PDT
WebKit Review Bot
Comment 72 2012-05-02 16:14:59 PDT
Comment on attachment 139873 [details] Patch Clearing flags on attachment: 139873 Committed r115907: <http://trac.webkit.org/changeset/115907>
WebKit Review Bot
Comment 73 2012-05-02 16:15:25 PDT
All reviewed patches have been landed. Closing bug.
Fady Samuel
Comment 74 2012-05-02 18:02:53 PDT
(In reply to comment #73) > All reviewed patches have been landed. Closing bug. If anyone still sees a problem here, please yell at me :-)
Csaba Osztrogonác
Comment 75 2012-05-02 22:45:50 PDT
Reopen, because it broke all viewport tests on Qt and on GTK. Qt results seems to be correct, but they needs to be updated: http://build.webkit.sed.hu/results/x86-64%20Linux%20Qt%20Release/r115937%20%2836886%29/results.html --- /ramdisk/qt-linux-64-release/build/layout-test-results/fast/viewport/viewport-1-expected.txt +++ /ramdisk/qt-linux-64-release/build/layout-test-results/fast/viewport/viewport-1-actual.txt @@ -1,2 +1,2 @@ -viewport size 320x352 scale 1.000000 with limits [1.000000, 5.000000] and userScalable -1.000000 +viewport size 320.000000x352.000000 scale 1.000000 with limits [1.000000, 5.000000] and userScalable -1.000000 But GTK results are absolutely false: http://build.webkit.org/results/GTK%20Linux%2064-bit%20Release/r115937%20%2823177%29/results.html --- /home/slave/webkitgtk/gtk-linux-64-release/build/layout-test-results/fast/viewport/viewport-1-expected.txt +++ /home/slave/webkitgtk/gtk-linux-64-release/build/layout-test-results/fast/viewport/viewport-1-actual.txt @@ -1,2 +1,2 @@ -viewport size 320x352 scale 1.000000 with limits [1.000000, 5.000000] and userScalable -1.000000 +viewport size -1082130432x480 scale 320.000000 with limits [352.000000, 1.000000] and userScalable 1.000000
Csaba Osztrogonác
Comment 76 2012-05-02 22:51:59 PDT
Rollout landed in http://trac.webkit.org/changeset/115939 Please update the expected files too with the proper fix.
Fady Samuel
Comment 77 2012-05-03 08:17:29 PDT
Fady Samuel
Comment 78 2012-05-03 08:28:58 PDT
Csaba Osztrogonác
Comment 79 2012-05-03 09:15:02 PDT
(In reply to comment #78) > Created an attachment (id=140021) [details] > Patch With this change one test result should be updated (at least on Qt): --- /home/oszi/WebKit/WebKitBuild/Release/layout-test-results/fast/viewport/viewport-86-expected.txt +++ /home/oszi/WebKit/WebKitBuild/Release/layout-test-results/fast/viewport/viewport-86-actual.txt @@ -1,2 +1,2 @@ -viewport size 457x503 scale 0.700000 with limits [0.700000, 5.000000] and userScalable -1.000000 +viewport size 457x502 scale 0.700000 with limits [0.700000, 5.000000] and userScalable -1.000000 Additionally please remove the tests from qt/Skipped lists you fixed with this patch: -# REGRESSION(r99195) -# https://bugs.webkit.org/show_bug.cgi?id=70609 -fast/viewport/viewport-83.html ... -fast/viewport/viewport-warnings-3.html -
Fady Samuel
Comment 80 2012-05-03 10:23:12 PDT
Martin Robinson
Comment 81 2012-05-03 10:40:26 PDT
Please unskip these tests on GTK+ as well. :)
Fady Samuel
Comment 82 2012-05-03 10:46:25 PDT
Fady Samuel
Comment 83 2012-05-03 10:48:21 PDT
(In reply to comment #81) > Please unskip these tests on GTK+ as well. :) Done. Landing...
WebKit Review Bot
Comment 84 2012-05-03 12:40:56 PDT
Comment on attachment 140047 [details] Patch Clearing flags on attachment: 140047 Committed r115998: <http://trac.webkit.org/changeset/115998>
WebKit Review Bot
Comment 85 2012-05-03 12:41:07 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.