Bug 70609 - Removing line in computeViewportAttributes that enforces a minimum scale factor to never allow zooming out more than viewport
Summary: Removing line in computeViewportAttributes that enforces a minimum scale fact...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Fady Samuel
URL:
Keywords:
Depends on: 85458
Blocks: 70559
  Show dependency treegraph
 
Reported: 2011-10-21 08:09 PDT by Fady Samuel
Modified: 2012-05-03 12:41 PDT (History)
17 users (show)

See Also:


Attachments
Patch (3.18 KB, patch)
2011-10-25 12:15 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (13.60 KB, patch)
2011-11-01 09:13 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (13.83 KB, patch)
2011-11-01 10:19 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (14.67 KB, patch)
2011-11-02 10:52 PDT, Fady Samuel
fsamuel: commit-queue+
Details | Formatted Diff | Diff
Patch for landing (10.19 KB, patch)
2011-11-03 07:30 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (14.61 KB, patch)
2011-11-03 08:15 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (14.67 KB, patch)
2011-11-03 08:19 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (2.41 KB, patch)
2012-04-24 11:04 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (3.03 KB, patch)
2012-05-02 08:12 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (4.38 KB, patch)
2012-05-02 08:31 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (5.28 KB, patch)
2012-05-02 09:36 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (7.72 KB, patch)
2012-05-02 10:06 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (7.72 KB, patch)
2012-05-02 11:20 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (7.78 KB, patch)
2012-05-02 12:09 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (9.04 KB, patch)
2012-05-02 13:42 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (15.62 KB, patch)
2012-05-03 08:17 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (15.45 KB, patch)
2012-05-03 08:28 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (17.27 KB, patch)
2012-05-03 10:23 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff
Patch (19.03 KB, patch)
2012-05-03 10:46 PDT, Fady Samuel
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Fady Samuel 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
Comment 1 Fady Samuel 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));
Comment 2 Fady Samuel 2011-10-25 12:15:29 PDT
Created attachment 112373 [details]
Patch
Comment 3 Kenneth Rohde Christiansen 2011-10-25 15:32:43 PDT
So basically what you want is the support for fit
Comment 4 Fady Samuel 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.
Comment 5 Kenneth Rohde Christiansen 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?
Comment 6 Daniel Bates 2011-10-25 16:39:53 PDT
Comment on attachment 112373 [details]
Patch

Attachment 112373 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/10219088
Comment 7 Fady Samuel 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.
Comment 8 Kenneth Rohde Christiansen 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.
Comment 9 zalan 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.
Comment 10 Fady Samuel 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.
Comment 11 Kenneth Rohde Christiansen 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.
Comment 12 Kenneth Rohde Christiansen 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.
Comment 13 zalan 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.
Comment 14 John Mellor 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.
Comment 15 Kenneth Rohde Christiansen 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.
Comment 16 Fady Samuel 2011-11-01 09:13:47 PDT
Created attachment 113176 [details]
Patch
Comment 17 Fady Samuel 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.
Comment 18 Konrad Piascik 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?
Comment 19 Fady Samuel 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;
Comment 20 Early Warning System Bot 2011-11-01 09:32:52 PDT
Comment on attachment 113176 [details]
Patch

Attachment 113176 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10244977
Comment 21 Gustavo Noronha (kov) 2011-11-01 09:33:59 PDT
Comment on attachment 113176 [details]
Patch

Attachment 113176 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10249315
Comment 22 Fady Samuel 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
Comment 23 Fady Samuel 2011-11-01 10:19:00 PDT
Created attachment 113186 [details]
Patch
Comment 24 Daniel Bates 2011-11-01 15:24:03 PDT
Comment on attachment 113186 [details]
Patch

Attachment 113186 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/10259052
Comment 25 Daniel Bates 2011-11-01 17:37:05 PDT
Comment on attachment 113186 [details]
Patch

Attachment 113186 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/10256096
Comment 26 Kenneth Rohde Christiansen 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.
Comment 27 zalan 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.
Comment 28 Kenneth Rohde Christiansen 2011-11-02 05:09:05 PDT
Comment on attachment 113186 [details]
Patch

Please make it work on mac become committing
Comment 29 Fady Samuel 2011-11-02 10:52:58 PDT
Created attachment 113333 [details]
Patch
Comment 30 Fady Samuel 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.
Comment 31 Fady Samuel 2011-11-03 07:30:23 PDT
Created attachment 113487 [details]
Patch for landing
Comment 32 Fady Samuel 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.
Comment 33 Fady Samuel 2011-11-03 08:15:45 PDT
Created attachment 113494 [details]
Patch
Comment 34 Fady Samuel 2011-11-03 08:19:21 PDT
Created attachment 113495 [details]
Patch
Comment 35 Fady Samuel 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>
Comment 36 Fady Samuel 2011-11-03 08:22:34 PDT
All reviewed patches have been landed.  Closing bug.
Comment 37 Csaba Osztrogonác 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’
Comment 38 Fady Samuel 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.
Comment 39 Csaba Osztrogonác 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.
Comment 40 Csaba Osztrogonác 2011-11-03 09:27:31 PDT
and unfortunately it broke 21 tests: http://build.webkit.org/results/Qt%20Linux%20Release/r99196%20%2839289%29/results.html
Comment 41 Csaba Osztrogonác 2011-11-03 09:52:19 PDT
Qt-WK2 buildfix landed in http://trac.webkit.org/changeset/99199
Comment 42 Csaba Osztrogonác 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
Comment 43 Fady Samuel 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.
Comment 44 Csaba Osztrogonác 2011-11-03 11:21:49 PDT
These tests fail on GTK too.
Comment 45 Philippe Normand 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!
Comment 46 Csaba Osztrogonác 2012-01-24 08:09:23 PST
This regression is still valid. Are you guys going to fix it?
Comment 47 Fady Samuel 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.
Comment 48 Philippe Normand 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?
Comment 49 Csaba Osztrogonác 2012-03-23 07:47:34 PDT
It is a 4.5 month old regression ... It isn't a fairplay to leave it unresolved.
Comment 50 Fady Samuel 2012-04-24 11:04:44 PDT
Created attachment 138600 [details]
Patch
Comment 51 Fady Samuel 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.
Comment 52 Build Bot 2012-04-24 11:30:52 PDT
Comment on attachment 138600 [details]
Patch

Attachment 138600 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12522279
Comment 53 Early Warning System Bot 2012-04-24 12:23:23 PDT
Comment on attachment 138600 [details]
Patch

Attachment 138600 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12525309
Comment 54 Early Warning System Bot 2012-04-24 13:36:34 PDT
Comment on attachment 138600 [details]
Patch

Attachment 138600 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12522301
Comment 55 Fady Samuel 2012-05-02 08:12:06 PDT
Created attachment 139817 [details]
Patch
Comment 56 Fady Samuel 2012-05-02 08:31:09 PDT
Created attachment 139824 [details]
Patch
Comment 57 Early Warning System Bot 2012-05-02 09:08:28 PDT
Comment on attachment 139824 [details]
Patch

Attachment 139824 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12593894
Comment 58 Early Warning System Bot 2012-05-02 09:16:11 PDT
Comment on attachment 139824 [details]
Patch

Attachment 139824 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12593897
Comment 59 Fady Samuel 2012-05-02 09:36:51 PDT
Created attachment 139829 [details]
Patch
Comment 60 Early Warning System Bot 2012-05-02 09:57:16 PDT
Comment on attachment 139829 [details]
Patch

Attachment 139829 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12595843
Comment 61 Early Warning System Bot 2012-05-02 09:59:31 PDT
Comment on attachment 139829 [details]
Patch

Attachment 139829 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12598731
Comment 62 Fady Samuel 2012-05-02 10:06:06 PDT
Created attachment 139835 [details]
Patch
Comment 63 Early Warning System Bot 2012-05-02 11:02:19 PDT
Comment on attachment 139835 [details]
Patch

Attachment 139835 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12604017
Comment 64 Early Warning System Bot 2012-05-02 11:03:24 PDT
Comment on attachment 139835 [details]
Patch

Attachment 139835 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12603016
Comment 65 Fady Samuel 2012-05-02 11:20:13 PDT
Created attachment 139842 [details]
Patch
Comment 66 Early Warning System Bot 2012-05-02 12:01:16 PDT
Comment on attachment 139842 [details]
Patch

Attachment 139842 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12609006
Comment 67 Early Warning System Bot 2012-05-02 12:04:30 PDT
Comment on attachment 139842 [details]
Patch

Attachment 139842 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12487038
Comment 68 Fady Samuel 2012-05-02 12:09:56 PDT
Created attachment 139855 [details]
Patch
Comment 69 Early Warning System Bot 2012-05-02 13:18:23 PDT
Comment on attachment 139855 [details]
Patch

Attachment 139855 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12611028
Comment 70 Early Warning System Bot 2012-05-02 13:35:41 PDT
Comment on attachment 139855 [details]
Patch

Attachment 139855 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12600042
Comment 71 Fady Samuel 2012-05-02 13:42:17 PDT
Created attachment 139873 [details]
Patch
Comment 72 WebKit Review Bot 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>
Comment 73 WebKit Review Bot 2012-05-02 16:15:25 PDT
All reviewed patches have been landed.  Closing bug.
Comment 74 Fady Samuel 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 :-)
Comment 75 Csaba Osztrogonác 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
Comment 76 Csaba Osztrogonác 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.
Comment 77 Fady Samuel 2012-05-03 08:17:29 PDT
Created attachment 140019 [details]
Patch
Comment 78 Fady Samuel 2012-05-03 08:28:58 PDT
Created attachment 140021 [details]
Patch
Comment 79 Csaba Osztrogonác 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
-
Comment 80 Fady Samuel 2012-05-03 10:23:12 PDT
Created attachment 140043 [details]
Patch
Comment 81 Martin Robinson 2012-05-03 10:40:26 PDT
Please unskip these tests on GTK+ as well. :)
Comment 82 Fady Samuel 2012-05-03 10:46:25 PDT
Created attachment 140047 [details]
Patch
Comment 83 Fady Samuel 2012-05-03 10:48:21 PDT
(In reply to comment #81)
> Please unskip these tests on GTK+ as well. :)

Done. Landing...
Comment 84 WebKit Review Bot 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>
Comment 85 WebKit Review Bot 2012-05-03 12:41:07 PDT
All reviewed patches have been landed.  Closing bug.